Closed Bug 1272409 Opened 9 years ago Closed 6 years ago

Implement Resize Observer API

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: dholbert, Assigned: boris)

References

(
URL
)

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [layout:backlog:2019q2:69][wptsync upstream])

Attachments

(6 files, 12 obsolete files)

Bug 1272409 - Part 1: Add GetNodeDepth() to nsContentUtils.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272409 - Part 2: Add ResizeObserver webidl and implementation.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272409 - Part 3: Add ResizeObserverController.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272409 - Part 4: Integrate ResizeObserver with Document and reflow.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272409 - Part 5: Update test meta.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272409 - Fix fuzzy unified build failed after adding new files into dom/base.
47 bytes, text/x-phabricator-request
Details | Review
The proposed Resize Observer API can be used to detect when elements are resized, and run some javascript in response (possibly causing further resizing), all between layout & paint. More details here: https://github.com/WICG/ResizeObserver [Note: this API is intentionally similar in "shape" to the Intersection Observer API, which is bug 1243846. --> Adding that bug as a "see also" here.]
Assignee: nobody → farislab
Status: NEW → ASSIGNED
Review Board Request already attached. The patches already implement all of the functionality of ResizeObserver and already passed all web-platform-test from https://github.com/WICG/ResizeObserver/tree/master/test. Somehow, web-platform-tests success in local but not in Try Server (See https://treeherder.mozilla.org/#/jobs?repo=try&revision=57344147775dc2ed3978a5b9bb76fb0015a19d85) Still investigating about test issues. Will add the web-platform-tests as part 5 of patches.
Comment on attachment 8789213 [details] Bug 1272409 part 1: Add ResizeObserver webidl and implementation. https://reviewboard.mozilla.org/r/77508/#review76040 Preliminary nits: ::: dom/base/ResizeObserver.h:113 (Diff revision 1) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObserverEntry) > + > + ResizeObserverEntry(nsISupports* aOwner, Element* aTarget) > + : mTarget(aTarget), mOwner(aOwner) Nit: it's a bit awkward that these member-variables are in the opposite order as compared to the arguments that this function takes. Consider making that consistent. The simplest fix is to just reverse the initialization list here, and to move the mOwner declaration up to be just before mTarget at the bottom of this class definition. ::: dom/base/ResizeObserver.h:123 (Diff revision 1) > + Constructor(const GlobalObject& aGlobal, > + Element* aTarget, > + mozilla::ErrorResult& aRv); > + > + virtual JSObject* WrapObject(JSContext* aCx, > + JS::Handle<JSObject*> aGivenProto) override Don't use "virtual" and "override" on the same method. Overriding methods are already virtual by definition, so "virtual" is already implied. (This is mentioned in the coding style guide -- search for "override" in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style for more details) (Our existing code isn't very good about this, since we only added this guideline in the last year or so I think. But, new code should follow it.) ::: dom/base/ResizeObserver.h:138 (Diff revision 1) > + > + Element* GetTarget() > + { > + return mTarget; > + } > + Whitespace on blank line here -- delete. ::: dom/base/ResizeObserver.h:156 (Diff revision 1) > + ~ResizeObserverEntry(); > + > + nsCOMPtr<Element> mTarget; > + RefPtr<DOMRectReadOnly> mContentRect; > + > + nsCOMPtr<nsISupports>mOwner; Add a space between ">" and "m". Also, per above, you might want to move this up to be declared just before "mTarget" for consistent ordering with the constructor's arguments. ::: dom/base/ResizeObserver.h:168 (Diff revision 1) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObservation) > + > + ResizeObservation(nsISupports* aOwner, Element* aTarget) > + : mBroadcastWidth(0), mBroadcastHeight(0), mTarget(aTarget), mOwner(aOwner) As above, consider reversing the init list entries for mOwner & mTarget here, and in their declarations at the bottom of this class definition. ::: dom/base/ResizeObserver.h:178 (Diff revision 1) > + Constructor(const GlobalObject& aGlobal, > + Element* aTarget, > + mozilla::ErrorResult& aRv); > + > + virtual JSObject* WrapObject(JSContext* aCx, > + JS::Handle<JSObject*> aGivenProto) override As above, drop "virtual" here (since it's implied by "override"). ::: dom/base/ResizeObserver.h:218 (Diff revision 1) > + > + /* > + * Returns the target's rect in the form of nsRect. > + * If the target is SVG, width and height are determined from bounding box. > + */ > + nsRect GetTargetRect( ); Delete space between () ::: dom/base/ResizeObserver.cpp:43 (Diff revision 1) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > + > +already_AddRefed<ResizeObserver> > +ResizeObserver::Constructor(const GlobalObject& aGlobal, > + ResizeObserverCallback& aCb, > + mozilla::ErrorResult& aRv) The last 2 lines are mis-indented here -- they should be lined up with "const" on the first line of the signature. (currently they're lined up with "GlobalObject") ::: dom/base/ResizeObserver.cpp:53 (Diff revision 1) > + if (!window) { > + aRv.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } > + > + MOZ_ASSERT(window->IsInnerWindow()); Please include a message in this assertion, explaining why we expect it to hold (and/or why it needs to hold). Assertions without explanatory messages should be rare, and should only happen when the reason for the assertion is totally obvious). ::: dom/base/ResizeObserver.cpp:54 (Diff revision 1) > + aRv.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } > + > + MOZ_ASSERT(window->IsInnerWindow()); > + Fix whitespace on blank line here and elsewhere in this file. (I see 4 instances -- MozReview helpfully highlights them in bright red.) ::: dom/base/ResizeObserver.cpp:106 (Diff revision 1) > + if (mObservationMap.Get(aTarget, getter_AddRefs(observation))) { > + mObservationMap.Remove(aTarget); > + > + MOZ_ASSERT(!mObservationList.isEmpty(), > + "If ResizeObservation found for an element, observation list" > + "must be not empty."); Nit -- add a space between |list| and the close-quote, or between the open-quote and |must|. Otherwise this message will print out with those words smooshed together, like "listmust". ::: dom/base/ResizeObserver.cpp:161 (Diff revision 1) > +ResizeObserver::BroadcastActiveObservations() > +{ > + uint32_t shallowestTargetDepth = 0; > + > + if (HasActiveObservations()) { > + Sequence<mozilla::OwningNonNull<ResizeObserverEntry>> entries; Drop the "mozilla::" prefix here -- you shouldn't need it, since this is already inside of namespace mozilla {... }. ::: dom/base/ResizeObserver.cpp:173 (Diff revision 1) > + RefPtr<ResizeObserverEntry> entry = > + new ResizeObserverEntry(this, target); > + > + entry->SetContentRect(observation->GetTargetRect()); > + > + *entries.AppendElement(mozilla::fallible) = entry; With the "mozilla::fallible" argument here, you're explicitly saying that AppendElement is allowed to fail (return null), if it needs to grow/reallocate its internal buffer and allocation fails. So: that means you shouldn't be directly dereferencing/assigning the returned value, because the return value might be null. This code needs to consider the possibility that AppendElement might fail, and do something reasonable in that case (maybe just break out of the loop right away, and figure it's too bad that we can't report all the remaining ones, and hopefully we'll recover from our low-memory situation). ::: dom/base/ResizeObserver.cpp:185 (Diff revision 1) > + > + if (targetDepth < shallowestTargetDepth || shallowestTargetDepth == 0) { > + shallowestTargetDepth = targetDepth; > + } > + > + } Drop the stray blank line between closing curly-braces here. ::: dom/base/ResizeObserver.cpp:281 (Diff revision 1) > + > +nsRect > +ResizeObservation::GetTargetRect() > +{ > + nsRect rect; > + nsIFrame* frame = GetTarget()->GetPrimaryFrame(Flush_Layout); Drop whitespace at the end of this line, and one instance further down in this method (after "Inline ||"). ::: dom/base/ResizeObserver.cpp:287 (Diff revision 1) > + > + if (frame) { > + if (GetTarget()->IsSVGElement()) { > + gfxRect bbox = nsSVGUtils::GetBBox(frame); > + rect.width = frame->PresContext()->GfxUnitsToAppUnits(bbox.width * 2); > + rect.height = frame->PresContext()->GfxUnitsToAppUnits(bbox.height * 2); Per our discussion, I think this "* 2" scale was because you're using dev pixels when you should be using CSS pixels, possibly. I think you want to use "NSFloatPixelsToAppUnits": https://dxr.mozilla.org/mozilla-central/rev/938ce16be25f9c551c19ef8938e8717ed3d41ff5/gfx/src/nsCoord.h#371 ...and probably pass in AppUnitsPerCSSPixel() (which is really "mozilla::AppUnitsPerCSSPixel()" but you can leave off the namespace).
Attachment #8789214 - Attachment is obsolete: true
Attachment #8789214 - Flags: review?(dholbert)
Attachment #8789215 - Attachment is obsolete: true
Attachment #8789215 - Flags: review?(dholbert)
Attachment #8789216 - Attachment is obsolete: true
Attachment #8789216 - Flags: review?(dholbert)
Attachment #8789213 - Attachment is obsolete: true
Attachment #8789213 - Flags: review?(dholbert)
Attachment #8789817 - Attachment is obsolete: true
Attachment #8789817 - Flags: review?(dholbert)
Attachment #8789819 - Attachment is obsolete: true
Attachment #8789819 - Flags: review?(dholbert)
Hi. I'm sorry for messing up the review request. I accidentally cancel the review request when I want to fix/add patch. Anyway, thanks for the preliminary review, Daniel! I also found the solution for test issues in Try Server before and have added the patch for web-platform-tests (part 5).
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review76666 Partial review for Part 1: ::: dom/base/ResizeObserver.h:29 (Diff revision 1) > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObserver) > + > + ResizeObserver(already_AddRefed<nsPIDOMWindowInner>&& aOwner, > + ResizeObserverCallback& aCb) > + : mOwner(aOwner), mCallback(&aCb) A few things: (1) already_AddRefed<>&& is a bit arcane. already_AddRefed is mostly intended to be used as a return value, not so much as an argument; and it's kinda deprecated by Move semantics, too. So: let's change this type to nsCOMPtr<nsPIDOMWindowInner>&&, and change the callers to use "Move(foo)" instead of "foo.forget()". (NOTE: it looks like this is based on nsDOMMutationObserver -- the usage of already_AddRefed there predated our usage of Move semantics, and we should probably separately upgrade it. I'll do that elsewhere.) (2) Can aOwner (and as a result mOwner) assumed to be non-null here? If so, please assert that in the body of the constructor -- e.g. MOZ_ASSERT(mOwner, "Need a non-null owner window"); (Ideally, we might want to wrap the type in NotNull<> all the way back to where this value comes from -- but assertions are probably better for now, given how much time you've got left. (3) Please indent the ":" (the init list) by 2 spaces here, to match the sample code here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes My 2nd and 3rd nits there apply to multiple places in the patch. ::: dom/base/ResizeObserver.h:44 (Diff revision 1) > + JS::Handle<JSObject*> aGivenProto) override > + { > + return ResizeObserverBinding::Wrap(aCx, this, aGivenProto); > + } > + > + nsISupports* GetParentObject() const Is this function assumed/guaranteed to return non-null? If so, it should be named "ParentObject()", without "Get". (This is to follow a particular convention that we have in Gecko code, for getters that return pointers. If a getter is allowed/expected to sometimes return null [and its callers need to worry about it possibly returning null], then the getter should be named "GetFoo()". And getters that *only* return something non-null are just called "Foo()". This makes it easier to reason about safe usage & whether nullchecks are needed at callsites.) ::: dom/base/ResizeObserver.h:68 (Diff revision 1) > + > + /* > + * Returns whether this ResizeObserver have any active observations > + * since last GatherActiveObservations(). > + */ > + bool HasActiveObservations(); Can this be "const"? Same for HasSkippedObservations below it. ::: dom/base/ResizeObserver.h:92 (Diff revision 1) > + mObservationList.clear(); > + } > + > + nsCOMPtr<nsPIDOMWindowInner> mOwner; > + > + RefPtr<ResizeObserverCallback> mCallback; If these mOwner and mCallback don't change after they're initialized in the constructor, then please declare them as "const". ::: dom/base/ResizeObserver.h:105 (Diff revision 1) > + // Will be nice if we have our own data structure for this in the future. > + nsRefPtrHashtable<nsPtrHashKey<Element>, ResizeObservation> mObservationMap; > + LinkedList<ResizeObservation> mObservationList; > +}; > + > +class ResizeObserverEntry final : public nsISupports, Please add a comment explaining how this class fits in with the other ResizeObserv* classes. (This file has ResizeObserver, ResizeObserverEntry, and ResizeObservation -- and it's not superficially clear how they differ & fit together. So, please document that, ideally in a short comment above each class, or perhaps in a larger comment up before the first one.) ::: dom/base/ResizeObserver.h:113 (Diff revision 1) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObserverEntry) > + > + ResizeObserverEntry(nsISupports* aOwner, Element* aTarget) > + : mOwner(aOwner), mTarget(aTarget) As above: (1) Please add assertions about mOwner & mTarget in the constructor body here, if we can assume they're non-null. (2) Indent ":" by 2 spaces. ::: dom/base/ResizeObserver.h:131 (Diff revision 1) > + aGivenProto); > + } > + > + nsISupports* GetParentObject() const > + { > + return mOwner; As above: I think this is guaranteed/assumed to return non-null? If so, it should be named "ParentObject". ::: dom/base/ResizeObserver.h:134 (Diff revision 1) > + nsISupports* GetParentObject() const > + { > + return mOwner; > + } > + > + Element* GetTarget() As above, drop the "Get". Also, this should be "const". ::: dom/base/ResizeObserver.h:141 (Diff revision 1) > + return mTarget; > + } > + > + /* > + * Returns the DOMRectReadOnly of target's content rect so it can be > + * accessed from Javascript in callback function of ResizeObserver. Nit: "Script" should be capitalized in JavaScript ::: dom/base/ResizeObserver.h:143 (Diff revision 1) > + > + /* > + * Returns the DOMRectReadOnly of target's content rect so it can be > + * accessed from Javascript in callback function of ResizeObserver. > + */ > + DOMRectReadOnly* ContentRect() In contrast to the above accessors, this one *does* need "Get", since (unlike the getters above it), it *can* return null, since mContentRect starts out null. So: please rename this "GetContentRect", and also mark it as "const". ::: dom/base/ResizeObserver.h:155 (Diff revision 1) > +protected: > + ~ResizeObserverEntry(); > + > + nsCOMPtr<nsISupports> mOwner; > + > + nsCOMPtr<Element> mTarget; Let's declare mElement and mOwner as "const", since (I think) their pointer values are never reassigned/modified after they get assigned in the constructor init list. (This, combined with the assertions I suggested above in the constructor, lets me feel more comfortable about assuming the accessors for these variables always return non-null.) ::: dom/base/ResizeObserver.h:157 (Diff revision 1) > + > + nsCOMPtr<nsISupports> mOwner; > + > + nsCOMPtr<Element> mTarget; > + > + RefPtr<DOMRectReadOnly> mContentRect; Let's drop the blank lines between the member-vars here. ::: dom/base/ResizeObserver.h:169 (Diff revision 1) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObservation) > + > + ResizeObservation(nsISupports* aOwner, Element* aTarget) > + : mOwner(aOwner), mTarget(aTarget), mBroadcastWidth(0), mBroadcastHeight(0) As above: (1) If aOwner and aTarget are guaranteed to be non-null, please add a MOZ_ASSERT to that effect. (2) indent init list by 2 spaces, to match coding style. ::: dom/base/ResizeObserver.h:184 (Diff revision 1) > + JS::Handle<JSObject*> aGivenProto) override > + { > + return ResizeObservationBinding::Wrap(aCx, this, aGivenProto); > + } > + > + nsISupports* GetParentObject() const As above, drop "Get". ::: dom/base/ResizeObserver.h:189 (Diff revision 1) > + nsISupports* GetParentObject() const > + { > + return mOwner; > + } > + > + Element* GetTarget() As above, drop "Get". Also, this (and I suspect all of the other methods besides UpdateBroadcastSize) can & should be marked as "const", I think -- BroadcastWidth/Height, IsActive, and GetTargetRect. ::: dom/base/ResizeObserver.h:226 (Diff revision 1) > +protected: > + ~ResizeObservation(); > + > + nsCOMPtr<nsISupports> mOwner; > + > + nsCOMPtr<Element> mTarget; As above, please declare mOwner and mTarget as "const", if they don't change after the constructor's init list. ::: dom/base/ResizeObserver.cpp:127 (Diff revision 1) > + mHasSkippedTargets = false; > + > + for (auto observation : mObservationList) { > + if (observation->IsActive()) { > + Element* target = observation->GetTarget(); > + MOZ_ASSERT(target, "Target of ResizeObservation shouldn't be null"); Once you've renamed observation->GetTarget() to just Target(), you can do away with this assertion here*. And really, it looks like you can shorten/simplify this code a bit by getting rid of "target" entirely, and instead just passing observation->GetTarget() directly at the point where |target| is used. * (the assertion isn't necessary (a) because it'll be implied by the naming of the method, and (b) because what it's asserting about will be reliably enforced elsewhere, by the assertion I'm suggesting that you add in |observation|'s constructor as well as the "const" label on its mTarget member-variable which means that assertion remains valid for the lifetime of the object) ::: dom/base/ResizeObserver.cpp:164 (Diff revision 1) > + Sequence<OwningNonNull<ResizeObserverEntry>> entries; > + > + for (auto observation : mActiveTargets) { > + > + Element* target = observation->GetTarget(); > + MOZ_ASSERT(target, "Target of ResizeObservation shouldn't be null"); As above, you can get rid of this assertion and this variable, and just pass "observation->Target()" directly at the one place where |target| is used here. ::: dom/base/ResizeObserver.cpp:210 (Diff revision 1) > + mOwner) > + > +already_AddRefed<ResizeObserverEntry> > +ResizeObserverEntry::Constructor(const GlobalObject& aGlobal, > + Element* aTarget, > + ErrorResult& aRv) Fix indentation here. ::: dom/base/ResizeObserver.cpp:220 (Diff revision 1) > +} > + > +void > +ResizeObserverEntry::SetContentRect(nsRect aRect) > +{ > + Element* target = GetTarget(); GetTarget() just returns mTarget. Seems like we should just get rid of this first line, and directly use "mTarget" in this function instead of creating a local variable. ::: dom/base/ResizeObserver.cpp:255 (Diff revision 1) > + mTarget, mOwner) > + > +already_AddRefed<ResizeObservation> > +ResizeObservation::Constructor(const GlobalObject& aGlobal, > + Element* aTarget, > + ErrorResult& aRv) Fix indentation here. ::: dom/base/ResizeObserver.cpp:281 (Diff revision 1) > + > +nsRect > +ResizeObservation::GetTargetRect() > +{ > + nsRect rect; > + nsIFrame* frame = GetTarget()->GetPrimaryFrame(Flush_Layout); s/GetTarget()/mTarget/ ::: dom/base/ResizeObserver.cpp:284 (Diff revision 1) > +{ > + nsRect rect; > + nsIFrame* frame = GetTarget()->GetPrimaryFrame(Flush_Layout); > + > + if (frame) { > + if (GetTarget()->IsSVGElement()) { s/GetTarget()/mTarget/ ::: modules/libpref/init/all.js:5551 (Diff revision 1) > #else > pref("dom.html_fragment_serialisation.appendLF", false); > #endif > + > +#ifdef NIGHTLY_BUILD > +pref("layout.css.resizeobserver.enabled", true); Please move this further up in this file, near other CSS features.
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review76666 > A few things: > (1) already_AddRefed<>&& is a bit arcane. already_AddRefed is mostly intended to be used as a return value, not so much as an argument; and it's kinda deprecated by Move semantics, too. So: let's change this type to nsCOMPtr<nsPIDOMWindowInner>&&, and change the callers to use "Move(foo)" instead of "foo.forget()". (NOTE: it looks like this is based on nsDOMMutationObserver -- the usage of already_AddRefed there predated our usage of Move semantics, and we should probably separately upgrade it. I'll do that elsewhere.) > > (2) Can aOwner (and as a result mOwner) assumed to be non-null here? If so, please assert that in the body of the constructor -- e.g. > MOZ_ASSERT(mOwner, "Need a non-null owner window"); > > (Ideally, we might want to wrap the type in NotNull<> all the way back to where this value comes from -- but assertions are probably better for now, given how much time you've got left. > > (3) Please indent the ":" (the init list) by 2 spaces here, to match the sample code here: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes > > My 2nd and 3rd nits there apply to multiple places in the patch. Sorry, please disregard my point (1) here (about already_AddRef) -- your usage of already_AddRef here is actually desirable, after all. It turns out there are subtle reasons that already_AddRefed is a good idea as a parameter, despite it being a bit verbose/arcane. None of those reasons come into play here, but from a consistent-coding-style perspective, it's probably better to stick with already_AddRefed here after all. (If you're curious why, see my question at https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/Np7oGnSyAAAJ and bz/khuey's responses for more details.) My points (2) and (3) are still valid, though. :)
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review76780 Here are a few more review comments. After you've addressed these, you should request review on this patch from smaug (Olli Pettay) -- he's a DOM peer, and he was involved with the design of MutationObserver & has written code in dom/base that's similar to this before, so he's the best person to have the final say on this patch (& perhaps some other patches here). ::: dom/base/ResizeObserver.h:206 (Diff revision 1) > + return mBroadcastHeight; > + } > + > + /* > + * Returns whether the observed target element size differ from > + * current broadcast width and broadcast height Add period at end of sentence. ::: dom/base/ResizeObserver.cpp:54 (Diff revision 1) > + aRv.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } > + > + nsCOMPtr<nsIDocument> document = window->GetExtantDoc(); > + MOZ_ASSERT(document); I'm not sure, offhand, whether (or why) we're justified in asserting that this variable, from GetExtantDocument(), is non-null here. If you're not sure either, I'd suggest replacing this assertion with a failure case like the one you have for !window. By default, you should treat GetFoo() getters as if they might return null, as I noted in a previous comment. (smaug might know for sure if we can assume this is null here, though. ::: dom/base/ResizeObserver.cpp:57 (Diff revision 1) > + > + nsCOMPtr<nsIDocument> document = window->GetExtantDoc(); > + MOZ_ASSERT(document); > + > + RefPtr<ResizeObserver> observer = new ResizeObserver(window.forget(), aCb); > + document->AddResizeObserver(observer); I don't think this function (AddResizeObserver, on nsIDocument) is declared/defined in this patch. So, I think this patch doesn't compile on its own, which we should fix. Please declare/define this function in this patch (though if you like, it's probably OK if it's got an empty implementation with a TODO comment, which gets filled in by a later patch). (Do please test to be sure that you can compile successfully at each intermediate point in your patch queue, too.) ::: dom/base/ResizeObserver.cpp:79 (Diff revision 1) > + observation = new ResizeObservation(this, aTarget); > + > + mObservationMap.Put(aTarget, observation); > + mObservationList.insertBack(observation); > + > + nsCOMPtr<nsIDocument> document = aTarget->OwnerDoc(); Do we actually need to hold onto a reference to |document| here? i.e. is it possible that we'll run script during this function which might destroy the document out from under us? I don't think that's a concern (as long as ScheduleResizeObserversNotification doesn't actually fire the callbacks), so I think we should probably do away with this |document| local variable and just replace its sole usage with |aTarget->OwnerDoc()->| (reference counting is not free, so we want to avoid putting things in local nsRefPtr/nsCOMPtr variables if we can guarantee that they'll stay alive through the function -- and we generally can guarantee that, as long as we're not running arbitrary scripts during the function.) ::: dom/base/ResizeObserver.cpp:84 (Diff revision 1) > + nsCOMPtr<nsIDocument> document = aTarget->OwnerDoc(); > + > + // Based on spec, we need to trigger notification in event loop that > + // contains ResizeObserver observe call even when resize/reflow does > + // not happen. > + document->ScheduleResizeObserversNotification(); As above, this function (ScheduleResizeObserversNotification on nsIDocument) needs to be declared/defined in this patch, so that we can compile successfully here. Please declare/define this function (even if it's just a stub with a TODO comment at this point). ::: dom/base/ResizeObserver.cpp:290 (Diff revision 1) > + gfxRect bbox = nsSVGUtils::GetBBox(frame); > + rect.width = NSFloatPixelsToAppUnits(bbox.width, AppUnitsPerCSSPixel()); > + rect.height = NSFloatPixelsToAppUnits(bbox.height, AppUnitsPerCSSPixel()); > + } else { > + // Non-replaced inline elements have fixed size so we will just let > + // the rect to (0,0,0,0) so no observation at non-replaced inline Question on this "0,0,0,0" hack: So, what's supposed to happen when a ResizeObserver is registered for a non-replaced element (say, a <div>), and that element *changes* from non-inline to inline? (e.g. someone toggles it from display:block to display:inline) Is that size change supposed to fire a resize? And is that actually what happens here? (I assume it should behave similarly to whatever happens for an element becoming "display:none".) Also: we should include a testcase for this scenario. Please add your own testcase (in addition to the ones from the spec) that tests our behavior in these situations (going from display:block to & from display:inline, as well as display:block to & from display:none) ::: dom/base/ResizeObserver.cpp:291 (Diff revision 1) > + rect.width = NSFloatPixelsToAppUnits(bbox.width, AppUnitsPerCSSPixel()); > + rect.height = NSFloatPixelsToAppUnits(bbox.height, AppUnitsPerCSSPixel()); > + } else { > + // Non-replaced inline elements have fixed size so we will just let > + // the rect to (0,0,0,0) so no observation at non-replaced inline > + // elements will be delivered This comment needs a little wordsmithing. (1) "have fixed size" isn't really true here -- these elements can & do change size, e.g. for font-size changes, or for wrapping at line boundaries (in which case they get split), etc. Let's just mention the spec's requirement, since that's the real reason we're skipping these elements here. So, maybe replace the first sentence with "We don't want to fire observations for non-replaced inline elements, per the spec". (2) s/let the rect to/leave the rect at/ (3) s/no observation at/no observations for/ (4) Add a period at the end of the paragraph. ::: dom/base/ResizeObserver.cpp:293 (Diff revision 1) > + } else { > + // Non-replaced inline elements have fixed size so we will just let > + // the rect to (0,0,0,0) so no observation at non-replaced inline > + // elements will be delivered > + if (frame->StyleDisplay()->mDisplay != StyleDisplay::Inline || > + frame->IsFrameOfType(nsIFrame::eReplaced)) { Two things: (1) I believe you really want to check for "eLineParticipant" here, rather than checking the computed "display" value. (There are other non-replaced inline "display" values beyond just "inline" -- e.g. various ruby-related display values. eLineParticipant should be the correct level of granularity.) Specifically, I think you want to check frame->IsFrameOfType(eLineParticipant) && !frame->IsFrameOfType(eReplaced)
Attachment #8789904 - Flags: review?(dholbert) → review-
Comment on attachment 8789905 [details] Bug 1272409 part 2: Add ResizeObserver webidl and implementation. https://reviewboard.mozilla.org/r/77952/#review76810 This patch looks good, but I should technically defer review to a /dom reviewer since it's entirely in /dom. --> over to smaug for an additional review. I'd call this r=me with the following, though (my last compactifying review nit being optional) ::: dom/base/nsContentUtils.h:2706 (Diff revision 1) > int32_t aNamespaceID, > nsIAtom* aAtom, > JS::MutableHandle<JSObject*> prototype); > > + /** > + * Returns the length of the parent-traversal path from node to root. ::: dom/base/nsContentUtils.h:2706 (Diff revision 1) > int32_t aNamespaceID, > nsIAtom* aAtom, > JS::MutableHandle<JSObject*> prototype); > > + /** > + * Returns the length of the parent-traversal path from node to root. s/node/aNode/ Also, please add a sentence along the lines of: "A root node (with no parent) is considered to be at a depth of 0." Also, per the assertion I'm suggesting below: please document that "aNode must be non-null" (assuming that matches your callsites' usage). ::: dom/base/nsContentUtils.cpp:9561 (Diff revision 1) > +/* static */ uint32_t > +nsContentUtils::GetNodeDepth(nsINode* aNode) > +{ > + uint32_t depth = 0; > + > + if (aNode) { Do other patches in this stack expect to call this for null aNode pointers? If not, let's document that (as noted above), and let's replace this null-check with a MOZ_ASSERT(aNode, ...) and proceed assuming that the passed-in value is non-null. ::: dom/base/nsContentUtils.cpp:9564 (Diff revision 1) > + uint32_t depth = 0; > + > + if (aNode) { > + nsINode* parent = aNode->GetParentNode(); > + > + while (parent) { If you like, the previous 4 lines can be collapsed (and "parent" can be removed entirely) to produce: while ((aNode = aNode->GetParentNode())) { (This is just doing the assignment in the "while" condition, and then treating the result as a boolean true/false value. The extra layer of parens is a compiler hint to say "yes, I really mean "=", not "=="; don't spam a compiler warning.) It's a bit more compact, but what you've got is OK too. ::: dom/base/nsContentUtils.cpp:9564 (Diff revision 1) > + uint32_t depth = 0; > + > + if (aNode) { > + nsINode* parent = aNode->GetParentNode(); > + > + while (parent) { If you like, the previous 4 lines can be collapsed (and "parent" can be removed entirely) to produce: while ((aNode = aNode->GetParentNode())) { (This is just doing the assignment in the "while" condition, and then treating the result as a boolean true/false value. The extra layer of parens is a compiler hint to say "yes, I really mean "=", not "=="; don't spam a compiler warning.) It's a bit more compact, but what you've got is OK too. ::: dom/base/nsContentUtils.cpp:9564 (Diff revision 1) > + uint32_t depth = 0; > + > + if (aNode) { > + nsINode* parent = aNode->GetParentNode(); > + > + while (parent) { If you like, the previous 4 lines can be collapsed to: while ((aNode = aNode->GetParentNode())) { Doing the assignment in the "while" condition, and then treating the result as a boolean true/false value. (The extra layer of parens is a compiler hint to say "yes, I really mean "=", not "=="; don't spam a compiler warning.) A bit more compact, but what you've got is OK too.
Attachment #8789905 - Flags: review?(dholbert) → review+
Attachment #8789905 - Flags: review?(bugs)
(Sorry, MozReview somehow created a duplicate empty-version of my first comment there, and preserved a duplicate draft of my last comment there. That'll teach me to use MozReview on the train with connectivity intermittently dropping. Anyway, hopefully comment 24 makes sense, with that clarification. :))
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review76812 ::: dom/base/ResizeObserverController.h:27 (Diff revision 3) > +{ > +public: > + NS_IMETHOD_(MozExternalRefCountType) AddRef(void) override; > + NS_IMETHOD_(MozExternalRefCountType) Release(void) override; > + > + NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(ResizeObserverNotificationHelper) (This can be replaced with NS_INLINE_DECL_REFCOUNTING -- though per IRC, I think you've already done that locally. Hooray! ::: dom/base/ResizeObserverController.h:30 (Diff revision 3) > + NS_IMETHOD_(MozExternalRefCountType) Release(void) override; > + > + NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(ResizeObserverNotificationHelper) > + > + explicit ResizeObserverNotificationHelper(nsIDocument* aDocument) > + : mDocument(aDocument), mRegistered(false) Add 2 spaces of indent before ":", and add an assertion that mDocument (and/or any other members that you're adding here which are expected to be non-null) are non-null. And as discussed today, if this object ends up needing a raw pointer back to the ResizeObserverController, please add a "Disconnect()" method which nulls out that pointer, and call that method from ~ResizeObserverController. ::: dom/base/ResizeObserverController.h:54 (Diff revision 3) > + > + bool mRegistered; > +}; > + > +/* > + * ResizeObserverController contain the list of ResizeObserver and control s/contain/contains/ s/ResizeObserver/ResizeObservers/ s/control/controls/ Also, while this class has a list of ResizeObservers, it doesn't actually *own* the ResizeObservers (it has raw pointers, i.e. non-owning references). Maybe it should own them? (I'm not immediately clear on who owns them.) ::: dom/base/ResizeObserverController.h:64 (Diff revision 3) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObserverController) > + > + explicit ResizeObserverController(nsIDocument* aDocument) > + : mDocument(aDocument), mIsNotificationActive(false) add spaces before ":", and assert that mDocument is non-null. ::: dom/base/ResizeObserverController.h:78 (Diff revision 3) > + } > + > + void AddResizeObserver(ResizeObserver* aObserver); > + > + /* > + * Notify all ResizeObserver and possibly call the broadcast active s/all ResizeObserver/all ResizeObservers/ (plural) Also, drop the word "possibly", I think, because this happens always, right? (It looks like the implementation calls GatherActiveObservationsAll and then BroadcastActiveObservationsAll, pretty much unconditionally.) Also, s/call the broadcast/broadcasts/ ::: dom/base/ResizeObserverController.h:86 (Diff revision 3) > + * be called after broadcast, causing the possibility of resize loop. > + */ > + void Notify(bool aReflow); > + > + /* > + * Calling GatherActiveObservations() for all ResizeObserver in this s/Calling/Calls/ s/all ResizeObserver/all ResizeObservers/ (plural) ::: dom/base/ResizeObserverController.h:89 (Diff revision 3) > + > + /* > + * Calling GatherActiveObservations() for all ResizeObserver in this > + * controller. > + */ > + void GatherActiveObservationsAll(uint32_t aDepth); Language-wise, the "All" suffix here doesn't make sense. For Gather...All and Broadcast...All, I think the "All" wants to be moved to the front (GatherAll... BroadcastAll...), OR you could just drop the "All" entirely, because it's kind of implied by the fact that this is the controller object that manages all the observers. ::: dom/base/ResizeObserverController.h:92 (Diff revision 3) > + * controller. > + */ > + void GatherActiveObservationsAll(uint32_t aDepth); > + > + /* > + * Calling BroadcastActiveObservations() for all ResizeObserver in this As above: s/Calling/Calls/ s/all ResizeObserver/all ResizeObservers/ (plural) ::: dom/base/ResizeObserverController.h:101 (Diff revision 3) > + uint32_t BroadcastActiveObservationsAll(); > + > + /* > + * Returns whether there is any ResizeObserver that has active observations. > + */ > + bool HasActiveObservationsAll(); Two things: (1) These "HasXYZ" functions look like they should be "const". (2) Language-wise, I think these methods really want to be named "HasAnyFOO" rather than "HasFOOAll()" -- i.e. they should be named "HasAnyActiveObservations()" and "HasAnySkippedObservations()". ::: dom/base/ResizeObserverController.h:118 (Diff revision 3) > + > +protected: > + virtual ~ResizeObserverController(); > + > + RefPtr<ResizeObserverNotificationHelper> mResizeObserverNotificationHelper; > + nsTArray<ResizeObserver*> mResizeObservers; Why are these raw (weak) pointers, & who actually owns reference-counted pointers to these mResizeObservers? Please add a code-comment here explaining that. (Raw-pointer member variables that point to temporary objects are scary -- they're use-after-free traps. They need some documentation to prove they're safe & that they outlive the actual thing that owns the references & keeps the objects alive.) ::: dom/base/ResizeObserverController.cpp:8 (Diff revision 3) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/dom/ErrorEvent.h" > +#include "mozilla/dom/ResizeObserverController.h" Please reverse the order of these #includes. (We have a (loosely-enforced) convention in Gecko code that Foo.cpp must have Foo.h as its *first* include. This increases the likelihood that Foo.h actually includes/declares everything that it uses.) ::: dom/base/ResizeObserverController.cpp:235 (Diff revision 3) > + mResizeObserverNotificationHelper->Register(); > +} > + > +ResizeObserverController::~ResizeObserverController() > +{ > + mResizeObservers.Clear(); This Clear() is unnecessary -- it'll happen automatically when the nsTArray member-variable gets destroyed.
Comment on attachment 8789906 [details] Bug 1272409 part 4: Integrate ResizeObserver with Document and reflow. https://reviewboard.mozilla.org/r/77954/#review76820 ::: dom/base/nsDocument.h:1205 (Diff revision 1) > > + void AddResizeObserver(mozilla::dom::ResizeObserver* aResizeObserver) override; > + > + void NotifyResizeObservers(bool aReflow) const override; > + > + void ScheduleResizeObserversNotification() const override; These functions should be added (as stubs if you like) in the earlier patches that add their callers, so that we can compile with those patches. So: AddResizeObserver and ScheduleResizeObserversNotification should be added in "part 1", and NotifyResizeObservers should be added in "part 3". ::: dom/base/nsDocument.h:1338 (Diff revision 1) > // fails and nothing gets changed. > bool ApplyFullscreen(const FullscreenRequest& aRequest); > > nsTArray<nsIObserver*> mCharSetObservers; > > + RefPtr<mozilla::dom::ResizeObserverController> mResizeObserverController; As discussed on IRC, I don't think this actually needs to be a refcounted object. We should store it in a nsAutoPtr or a mozilla::UniquePtr (instead of RefPtr), and replace its refcounted mDocument pointer with a raw pointer, and give it a "Disconnect()" method that clears out that raw pointer, and add a call to that Disconnect method in ~nsDocument() (like the disconnect call that we make for mAnimationController, for example). That should mean we can get rid of a lot of refcounting/cycle-collection macros, I think -- and ownership/lifetimes will be easier to follow & reason about.
Addressed all review comments (except for adding the test case for non-replaced inline elements) with the revised patches. Some of review concerns already discussed on IRC, but I will mention some of them again in here. (In reply to Daniel Holbert [:dholbert] from comment #21) > ::: dom/base/ResizeObserver.h:44 > (Diff revision 1) > > + JS::Handle<JSObject*> aGivenProto) override > > + { > > + return ResizeObserverBinding::Wrap(aCx, this, aGivenProto); > > + } > > + > > + nsISupports* GetParentObject() const > > Is this function assumed/guaranteed to return non-null? If so, it should be > named "ParentObject()", without "Get". The Binding class that generated by WebIDL Binding calls GetParentObject() so I think we should stick with it in here. > ::: dom/base/ResizeObserver.h:92 > (Diff revision 1) > > + mObservationList.clear(); > > + } > > + > > + nsCOMPtr<nsPIDOMWindowInner> mOwner; > > + > > + RefPtr<ResizeObserverCallback> mCallback; > > If these mOwner and mCallback don't change after they're initialized in the > constructor, then please declare them as "const". If we declare them (and also some other members in ResizeObserverEntry and ResizeObservation) as const, they will cause some errors in cycle collection macros. (In reply to Daniel Holbert [:dholbert] from comment #23) > ::: dom/base/ResizeObserver.cpp:290 > (Diff revision 1) > > + gfxRect bbox = nsSVGUtils::GetBBox(frame); > > + rect.width = NSFloatPixelsToAppUnits(bbox.width, AppUnitsPerCSSPixel()); > > + rect.height = NSFloatPixelsToAppUnits(bbox.height, AppUnitsPerCSSPixel()); > > + } else { > > + // Non-replaced inline elements have fixed size so we will just let > > + // the rect to (0,0,0,0) so no observation at non-replaced inline > > Question on this "0,0,0,0" hack: > > So, what's supposed to happen when a ResizeObserver is registered for a > non-replaced element (say, a <div>), and that element *changes* from > non-inline to inline? (e.g. someone toggles it from display:block to > display:inline) > > Is that size change supposed to fire a resize? And is that actually what > happens here? > > (I assume it should behave similarly to whatever happens for an element > becoming "display:none".) > > Also: we should include a testcase for this scenario. Please add your own > testcase (in addition to the ones from the spec) that tests our behavior in > these situations (going from display:block to & from display:inline, as well > as display:block to & from display:none) Yes, in this implementation, it will fire an observation. The spec says to watching content width and content height (https://wicg.github.io/ResizeObserver/#content-rect). In https://www.w3.org/TR/CSS2/visudet.html#propdef-width, content width and content height does not apply to non-replaced inline elements (that's why the ResizeObserver spec specify that observation do not fire for non-replaced inline Elements). So, in this case, the content width and content height changed from applied to non-applied, which should trigger the observation. I need some clarification about this 'applied' means in our platform implementation and I think we also need to ask for clarification to spec author regarding this case. Actually, it would be nice if we can only call the function that will returns the desired content rect. GetClientAreaRect() in http://searchfox.org/mozilla-central/source/dom/base/Element.cpp#951 return nsRect(0, 0, 0, 0) for non-replaced inline element but ClientAreaRect is not the desired rect for our case in ResizeObserver. Maybe we can make a pull request to ResizeObserver Github repository to add a test case for a non-inline element that changed to inline element (and also make an Issue for it for clarification). (In reply to Daniel Holbert [:dholbert] from comment #26) > ::: dom/base/ResizeObserverController.h:118 > (Diff revision 3) > > + > > +protected: > > + virtual ~ResizeObserverController(); > > + > > + RefPtr<ResizeObserverNotificationHelper> mResizeObserverNotificationHelper; > > + nsTArray<ResizeObserver*> mResizeObservers; > > Why are these raw (weak) pointers, & who actually owns reference-counted > pointers to these mResizeObservers? Please add a code-comment here > explaining that. (Raw-pointer member variables that point to temporary > objects are scary -- they're use-after-free traps. They need some > documentation to prove they're safe & that they outlive the actual thing > that owns the references & keeps the objects alive.) As discussed on IRC, seems like this is done by generated code from WebIDL Binding.
(In reply to Fariskhi Vidyan [:fvidyan] from comment #34) > Addressed all review comments (except for adding the test case for > non-replaced inline elements) with the revised patches. Thanks! You should mark them as "Fixed" in mozreview, too -- otherwise, they stick around there forever and will get mixed up with further review comments. (I think the idea is that you can mark them as "fixed" as you fix them locally, or something like that, to be sure you don't miss any of them. But if you've addressed them all, you should be able to go through & click "fixed" on all of 'em.) > (In reply to Daniel Holbert [:dholbert] from comment #23) > > So, what's supposed to happen when a ResizeObserver is registered for a > > non-replaced element [...] > > Yes, in this implementation, it will fire an observation. [...] > I think we also need to ask for clarification to > spec author regarding this case. OK. I filed https://github.com/WICG/ResizeObserver/issues/19 > Maybe we can make a pull request to ResizeObserver Github repository to add > a test case for a non-inline element that changed to inline element (and > also make an Issue for it for clarification). That would probably, be good, yeah. But, in the interests of getting this landed, let's include that test here first, so we can have better code-coverage of edge cases, and then we can uplift the test to the spec's github repo (& perhaps address feedback from the spec author) afterwards. > (In reply to Daniel Holbert [:dholbert] from comment #26)eObserverNotificationHelper; > > > + nsTArray<ResizeObserver*> mResizeObservers; > > > > Why are these raw (weak) pointers, & who actually owns reference-counted > > pointers to these mResizeObservers? > > As discussed on IRC, seems like this is done by generated code from WebIDL > Binding. OK. I'm a little wary that the only thing holding onto these objects *might* be the "observer" variables in JavaScript -- and they might only be getting cleaned up because our current testcases are nice about unregistering. You should try setting "observer = null" without ever unregistering it (to drop JS's only reference to the observer), and then clicking "GC" and "CC" buttons in an about:memory tab, and then triggering some resizes. I'm worried that that might crash, if the JS "observer" variables are holding the only reference to these things.
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review76938 ::: dom/base/ResizeObserverController.h:12 (Diff revision 4) > +#ifndef mozilla_dom_ResizeObserverController_h > +#define mozilla_dom_ResizeObserverController_h > + > +#include "mozilla/dom/ResizeObserver.h" > +#include "mozilla/TimeStamp.h" > +#include "nsCycleCollectionParticipant.h" I think you can get rid of this cycle-collection #include -- you don't seem to be using it here anymore. ::: dom/base/ResizeObserverController.h:22 (Diff revision 4) > + > +class ResizeObserverController; > + > +/* > + * ResizeObserverNotificationHelper will trigger ResizeObserver notification > + * using a RefreshObserver. s/notification/notifications/ (since there could be more than one notification) s/using a RefreshObserver/by registering with the Refresh Driver/ ("using a" sounds like we're using some *other* object as the refresh observer But really, this object is the nsARefreshObserver itself.) ::: dom/base/ResizeObserverController.h:66 (Diff revision 4) > + : mDocument(aDocument) > + , mIsNotificationActive(false) > + { > + MOZ_ASSERT(mDocument, "Need a non-null document"); > + mResizeObserverNotificationHelper = new > + ResizeObserverNotificationHelper(this); Nit: wrap to a new line *before* "new" (not after it), so that "new Whatever()" is all on the same line. ::: dom/base/ResizeObserverController.h:113 (Diff revision 4) > + * Returns whether there is any ResizeObserver that has skipped observations. > + */ > + bool HasAnySkippedObservations() const; > + > +protected: > + nsIDocument* const mDocument; Please include a comment to justify the raw pointer usage here (and how we know it'll stay alive for as long as we're using it). Something like this: // Raw pointer OK b/c mDocument strongly owns us & hence must outlive us. ::: dom/base/ResizeObserverController.h:114 (Diff revision 4) > + */ > + bool HasAnySkippedObservations() const; > + > +protected: > + nsIDocument* const mDocument; > + bool mIsNotificationActive; Move this bool a few lines down, to be the final member variable declared here. (We try to put smaller (possibly-less-than-a-word-in-size) member variables at the end, since it encourages better packing of memory with fewer "holes" in the middle of a struct.) ::: dom/base/ResizeObserverController.h:120 (Diff revision 4) > + > + RefPtr<ResizeObserverNotificationHelper> mResizeObserverNotificationHelper; > + > + // WebIDL generated code keep ResizeObserver object alive with strong > + // reference and outlive the Document (which is the owner of > + // ResizeObserverController object) so it's save to use Raw Pointer of Nits on this comment: s/keep/keeps/ s/save/safe/ s/Raw Pointer of/raw pointers to/ ... though per end of comment 35, I'm not sure whether this comment is actually true or not, so maybe this will need to go away. (FWIW I just talked to tobytailor, & he says he thinks his analogous array-of-raw-pointers is problematic (in the IntersectionObserver patches), and he's working on fixing his version of this issue.) ::: dom/base/ResizeObserverController.cpp:9 (Diff revision 4) > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/dom/ResizeObserverController.h" > +#include "mozilla/dom/ErrorEvent.h" > +#include "nsCycleCollectionParticipant.h" As in the .h file, I think you can get rid of this cycle-collection #include now -- you don't seem to be using it here anymore. ::: dom/base/ResizeObserverController.cpp:49 (Diff revision 4) > + if (mRegistered) { > + return; > + } > + > + nsRefreshDriver* refreshDriver = GetRefreshDriver(); > + MOZ_ASSERT(refreshDriver, "RefreshDriver shouldn't be null"); This "shouldn't be null" assertion is problematic, because: (1) GetRefreshDriver is spelled with a "Get" (which suggests it can return null) (2) and indeed GetRefreshDriver does have two explicit "return nullptr" cases. If an assertion were merited here, we'd use a non-fatal assertion like NS_ASSERTION(...), since we shouldn't abort if we know it's something that can happen. But really, we should probably just replace the assertion with a null-check, and add a comment like... // We must be navigating away from this page, or something ...and just fail gracefully (probably not setting mRegistered to true). ::: dom/base/ResizeObserverController.cpp:63 (Diff revision 4) > + if (!mRegistered) { > + return; > + } > + > + nsRefreshDriver* refreshDriver = GetRefreshDriver(); > + MOZ_ASSERT(refreshDriver, "RefreshDriver shouldn't be null"); As above, we can't really justify this assertion; it should be a null-check instead. ::: dom/base/ResizeObserverController.cpp:73 (Diff revision 4) > + > +void > +ResizeObserverNotificationHelper::Disconnect() > +{ > + Unregister(); > + mOwner = nullptr; Add a comment to explain the pointer-clearing here, like: // Our owner is dying. Clear our pointer to it, in case we outlive it. ::: dom/base/ResizeObserverController.cpp:78 (Diff revision 4) > + mOwner = nullptr; > +} > + > +ResizeObserverNotificationHelper::~ResizeObserverNotificationHelper() > +{ > + Disconnect(); In the Helper's own destructor, we should just call Unregister() (not Disconnect). (There's no need to clear the raw pointer to our controller, when this object (the helper) is being destructed. In that case, our memory is about to get potentially-stomped on anyway, and no other code should be caring what state we leave it in. We only need to clear our raw pointer via Disconnect() when *the thing we're pointing to* (the controller) is dying, because we're not necessarily sure the helper object will die right away after that, so there's a chance the raw pointer might get used in that case.) ::: dom/base/ResizeObserverController.cpp:84 (Diff revision 4) > +} > + > +void > +ResizeObserverController::AddResizeObserver(ResizeObserver* aObserver) > +{ > + mResizeObservers.AppendElement(aObserver); Please assert that |aObserver| is non-null here, before we add it to the array. (A fatal MOZ_ASSERT would be appropriate here, because we don't expect this to ever happen -- but if it did happen, then we're going to crash soon anyway when we iterate over mResizeObservers, so we might as well abort debug-builds a bit earlier with a helpful error message.) ::: dom/base/ResizeObserverController.cpp:90 (Diff revision 4) > +} > + > +void > +ResizeObserverController::Notify() > +{ > + if (mResizeObservers.IsEmpty() || mIsNotificationActive) { I don't think you need to check mIsNotificationActive here anymore, since you're not making Reflow call Notify synchronously anymore. (though you did in earlier patches) So: please remove this mIsNotificationActive check, and replace it with an *assertion* that mIsNotificationActive is false. (either NS_ASSERTION or MOZ_ASSERT should both be fine here, I think) (I was going to suggest removing the variable entirely, but I think it still serves a purpose -- as noted below, I think you should check it in ScheduleNotification().) ::: dom/base/ResizeObserverController.cpp:95 (Diff revision 4) > + if (mResizeObservers.IsEmpty() || mIsNotificationActive) { > + return; > + } > + > + uint32_t shallowestTargetDepth = 0; > + mIsNotificationActive = true; Just before you assign this to "true", let's declare an RAII auto-restore object, like so: AutoRestore<bool> autoRestore(mIsNotificationActive); That will automatically restore the old value of the boolean when this function returns (even if we return via some hypothetical future early-return codepath -- we don't need to add an extra "false" assignment for that early-return codepath). ::: dom/base/ResizeObserverController.cpp:100 (Diff revision 4) > + mIsNotificationActive = true; > + > + GatherAllActiveObservations(shallowestTargetDepth); > + > + while (HasAnyActiveObservations()) { > + shallowestTargetDepth = BroadcastAllActiveObservations(); Let's add some debug-only code to sanity-check the invariant that shallowestTargetDepth is getting strictly deeper (which I think it should be? That's how this loop is supposed to terminate, IIUC.) Something like this (I'm not 100% sure about the logic but hopefully you can adjust to make this correct): DebugOnly<uint32_t> oldShallowestTargetDepth = shallowestTargetDepth; shallowestTargetDepth = BroadcastAllActiveObservations(); NS_ASSERTION(oldShallowestTargetDepth < shallowestTargetDepth, "shallowestTargetDepth should be getting strictly deeper"); ::: dom/base/ResizeObserverController.cpp:103 (Diff revision 4) > + > + while (HasAnyActiveObservations()) { > + shallowestTargetDepth = BroadcastAllActiveObservations(); > + > + // Flush layout will trigger reflow to check for possibility of > + // resize loop but will not do the notification again because This comment could be reworded a bit (in part because it's not immediately clear what sort of "resize loop" you're talking about checking for here). Maybe something like: // Flush layout, so that any callback functions' style changes / resizes // get a chance to take effect. ::: dom/base/ResizeObserverController.cpp:113 (Diff revision 4) > + // that have the depth of observed target element more than current > + // shallowestTargetDepth. > + GatherAllActiveObservations(shallowestTargetDepth); > + } > + > + mIsNotificationActive = false; With the "autoRestore" suggested above, you can get rid of this "= false" line, since it'll happen automagically. (It'll happen a bit later -- when we return -- but that's fine, I think.) ::: dom/base/ResizeObserverController.cpp:119 (Diff revision 4) > + > + if (HasAnySkippedObservations()) { > + RootedDictionary<ErrorEventInit> init(RootingCx()); > + > + init.mMessage.AssignLiteral( > + "ResizeObserver loop completed with undelivered notifications."); Please add a comment at the top of this block to give some context for this JS error (and in particular, you should say that it's something required by the spec, rather than e.g. a one-off thing that we're posting an error about just because we feel like it might be merited). e.g.: // Per spec, we deliver an error if the document has any skipped notifications. // https://wicg.github.io/ResizeObserver/#deliver-resize-error ::: dom/base/ResizeObserverController.cpp:153 (Diff revision 4) > +} > + > +uint32_t > +ResizeObserverController::BroadcastAllActiveObservations() > +{ > + uint32_t shallowestTargetDepth = 0; The spec says to initialize this to +infinity, rather than to 0. Let's set it to UINT32_MAX to better-match the spec, and to make the algorithm easier to follow. (And because I think there's a bug if we use "0" as a sentinel; see my next review-comment about the "== 0" check for details) ::: dom/base/ResizeObserverController.cpp:159 (Diff revision 4) > + > + for (auto observer : mResizeObservers) { > + > + uint32_t targetDepth = observer->BroadcastActiveObservations(); > + > + if (targetDepth < shallowestTargetDepth || shallowestTargetDepth == 0) { You can drop the second condition here (for "== 0"), now that you'll be starting this variable off at +infinity rather than at 0. (And actually, I think the "special treatment for 0" might've made us susceptible to bugs, in this current patch. Suppose our first call to BroadcastActiveObservations() legitimately returns 0 (i.e. the root node was being observed, and it got resized), and the next BroadcastActiveObservations() call returns a larger value. In that scenario, the current patch's code here would've had us use the larger value due to this "== 0" check, when really we should've stuck with the shallower 0 value. Maybe this can't happen because maybe 0 would never actually be returned (not sure), but it's a theoretical concern at least.) ::: dom/base/ResizeObserverController.cpp:192 (Diff revision 4) > +} > + > +void > +ResizeObserverController::ScheduleNotification() > +{ > + mResizeObserverNotificationHelper->Register(); I suspect we should check "mIsNotificationActive" before calling Register() here, to prevent Notify()-triggered "resize loops" from getting unnecessary refresh driver registrations (which are unnecessary because we're actually handling those resizes immediately as part of the "resize loop" that we're currently cycling through, if mIsNotificationActive is true)
Attachment #8789818 - Flags: review?(dholbert) → review-
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review76780 > Question on this "0,0,0,0" hack: > > So, what's supposed to happen when a ResizeObserver is registered for a non-replaced element (say, a <div>), and that element *changes* from non-inline to inline? (e.g. someone toggles it from display:block to display:inline) > > Is that size change supposed to fire a resize? And is that actually what happens here? > > (I assume it should behave similarly to whatever happens for an element becoming "display:none".) > > Also: we should include a testcase for this scenario. Please add your own testcase (in addition to the ones from the spec) that tests our behavior in these situations (going from display:block to & from display:inline, as well as display:block to & from display:none) Alex replied on the github issue ( https://github.com/WICG/ResizeObserver/issues/19 ) and confirmed that we should *not* fire the callback when an element changes from block-level to inline-level (which makes some sense, because as he notes, the size might or might not actually be changing). I guess inline-level elements simply don't exist from the perspective of ResizeObservers, kind of. (but in a different way than how "display:none" elements don't exist) So anyway -- this will indeed need some tweaking to match the spec here.
Comment on attachment 8789905 [details] Bug 1272409 part 2: Add ResizeObserver webidl and implementation. https://reviewboard.mozilla.org/r/77952/#review77012 One last nit on this part, and then r=me, but this should also get review from smaug -- please include "r=dholbert r?smaug" in the commit message when you reupload, and mozreview should automatically tag him, I think. ::: dom/base/nsContentUtils.h:2707 (Diff revision 2) > nsIAtom* aAtom, > JS::MutableHandle<JSObject*> prototype); > > + /** > + * Returns the length of the parent-traversal path from aNode to root. > + * A root node (with no parent) is considered to be at a depth of 0. Please add at the end of the header-documentation here: aNode is expected to be non-null. (Sorry, I should've made it clearer in comment 24 that I was looking for an assertion *as well as* some documentation in the header file to warn callers about the assertion/expectation.)
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review77020 Nits on the updated Part 1: ::: dom/base/ResizeObserver.h:60 (Diff revision 2) > + void Disconnect(); > + > + /* > + * Gather all observations which have an observed target with size changed > + * since last BroadcastActiveObservations() in this ResizeObserver. > + * Observation will be skipped if the depth of observed target is less or s/Observation/An observation/ s/depth of observed/depth of its observed/ ::: dom/base/ResizeObserver.h:81 (Diff revision 2) > + bool HasSkippedObservations() const; > + > + /* > + * Deliver the callback function in JavaScript for all active observations > + * and pass the sequence of ResizeObserverEntry so JavaScript can access them. > + * Broadcast size of observations will be updated and mActiveTargets will be s/Broadcast size/The broadcast size/ (Otherwise this is ambiguous, since "Broadcast" is a verb -- so it kinda sounds like this sentence is saying "[This function will] broadcast [the] size...". But really it means to say "The broadcast size will be updated") ::: dom/base/ResizeObserver.h:108 (Diff revision 2) > +}; > + > +/** > + * ResizeObserverEntry is the entry that contains the information for observed > + * elements. This object is the one that visible to JavaScript in callback > + * function that fired by ResizeObserver. s/that fired/that is fired/ ::: dom/base/ResizeObserver.h:223 (Diff revision 2) > + { > + return mBroadcastHeight; > + } > + > + /* > + * Returns whether the observed target element size differ from s/differ/differs/ ::: dom/base/ResizeObserver.cpp:156 (Diff revision 2) > +} > + > +uint32_t > +ResizeObserver::BroadcastActiveObservations() > +{ > + uint32_t shallowestTargetDepth = 0; As noted for another patch, should we start out |shallowestTargetDepth| at UINT32_MAX instead of 0? (and drop the "shallowestTargetDepth == 0" special case below) Then we can let it just strictly get smaller, and I think we'll be more closely following the spec. ::: dom/base/ResizeObserver.cpp:218 (Diff revision 2) > + > +void > +ResizeObserverEntry::SetContentRect(nsRect aRect) > +{ > + RefPtr<DOMRect> contentRect = new DOMRect(mTarget); > + nsIFrame* frame = mTarget->GetPrimaryFrame(Flush_Layout); I think this just wants to be GetPrimaryFrame(). (No need to force a Flush_Layout.) A layout flush here would be potentially expensive (though it would probably just be spinning our wheels, most of the time). This function gets called for every observation that we're broadcasting, it looks like, and I don't think we need to independently flush layout for each of those. If we're broadcasting notifications, then presumably layout has already been flushed, I think? So, GetPrimaryFrame() with no flush should be fine. ::: dom/base/ResizeObserver.cpp:277 (Diff revision 2) > + > +nsRect > +ResizeObservation::GetTargetRect() const > +{ > + nsRect rect; > + nsIFrame* frame = mTarget->GetPrimaryFrame(Flush_Layout); As above, I suspect we just want GetPrimaryFrame() here (no Flush_Layout), as long as layout has already been flushed before this function is called (e.g. via IsActive()) Otherwise this function ends up being deceptively expensive/complex.
Attachment #8789904 - Flags: review?(dholbert) → review-
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review76938 > I don't think you need to check mIsNotificationActive here anymore, since you're not making Reflow call Notify synchronously anymore. (though you did in earlier patches) > > So: please remove this mIsNotificationActive check, and replace it with an *assertion* that mIsNotificationActive is false. (either NS_ASSERTION or MOZ_ASSERT should both be fine here, I think) > > (I was going to suggest removing the variable entirely, but I think it still serves a purpose -- as noted below, I think you should check it in ScheduleNotification().) I think we can remove the variable entirely. Maybe we can call mOwner->Notify() first and then Unregister() in ResizeObserverNotificationHelper::WillRefresh() so when Reflow call ScheduleResizeObserversNotification(), it will not register the helper because mRegistered already true. > Let's add some debug-only code to sanity-check the invariant that shallowestTargetDepth is getting strictly deeper (which I think it should be? That's how this loop is supposed to terminate, IIUC.) > > > Something like this (I'm not 100% sure about the logic but hopefully you can adjust to make this correct): > > DebugOnly<uint32_t> oldShallowestTargetDepth = shallowestTargetDepth; > shallowestTargetDepth = BroadcastAllActiveObservations(); > NS_ASSERTION(oldShallowestTargetDepth < shallowestTargetDepth, > "shallowestTargetDepth should be getting strictly deeper"); Since the initializaion of shallowestTargetDepth = UINT32_MAX, I think we should check for (oldShallowestTargetDepth < shallowestTargetDepth || oldShallowestTargetDepth == UINT32_MAX) in the assertion.
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review76938 > Since the initializaion of shallowestTargetDepth = UINT32_MAX, I think we should check for (oldShallowestTargetDepth < shallowestTargetDepth || oldShallowestTargetDepth == UINT32_MAX) in the assertion. Ups, sorry. The shallowestTargetDepth variable is initialized to 0 at the beginning of Notify(). The one that will be initialized to UINT32_MAX is in BroadcastAllActiveObservations() so your assertion suggestion already correct.
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review76812 > Why are these raw (weak) pointers, & who actually owns reference-counted pointers to these mResizeObservers? Please add a code-comment here explaining that. (Raw-pointer member variables that point to temporary objects are scary -- they're use-after-free traps. They need some documentation to prove they're safe & that they outlive the actual thing that owns the references & keeps the objects alive.) Changed to nsTArray<RefPtr<ResizeObserver>> and add the cycle collection support for that in ResizeObserverController
(Side note: at my request, Fariskhi swapped the order of parts 1 and 2 here -- I'd asked for that since the old part 1, "Add ResizeObserver webidl and implementation", kinda depended on a function call to something that wasn't being added until the old part 2, "Add GetNodeDepth() to nsContentUtils." So, they make more sense in the new ordering. I'm mentioning this because the MozReview history is a bit mixed up for them now, though, because as part of swapping/updating the patches, the MozReview-Commit-IDs got reset -- so MozReview is viewing the new Part 1 as being an evolution of the old Part 1 rather than the old Part 2, and vice versa. So, the patch title at the beginning of comment 39 is now incorrect (because mozreview auto-updated that patch title). This sort of thing is covered by bug 1301532. Anyway, not a huge deal here, since review feedback has been addressed and I'm not particularly concerned with interdiffs up to this point. I'm just mentioning it as a historical note, in case someone's wondering why e.g. comment 39 & comment 24 seem to be pointing at the wrong patch.)
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review77652 This GetNodeDepth patch looks good to me -- r+ (though smaug should still sign off on it as well). (Side note: there's a github issue open about the spec details of GetNodeDepth, here: https://github.com/WICG/ResizeObserver/issues/21#issuecomment-247113919 Though the spec text is still being sorted out, I think this implementation of the algorithm matches the intent behind the spec, so it looks good to me.)
Attachment #8789904 - Flags: review?(dholbert) → review+
Comment on attachment 8789905 [details] Bug 1272409 part 2: Add ResizeObserver webidl and implementation. https://reviewboard.mozilla.org/r/77952/#review77656 r=me, and smaug should take a look at this now. ::: dom/base/ResizeObserver.h:99 (Diff revision 3) > + RefPtr<ResizeObserverCallback> mCallback; > + nsTArray<RefPtr<ResizeObservation>> mActiveTargets; > + bool mHasSkippedTargets; > + > + // Combination of HashTable and LinkedList so we can iterate through > + // the elements of HashTable in order of insertion time. Probably worth hinting *why* we care about iterating in order of insertion time here. Maybe add at the end of this sentence: (so we can deliver observations in the correct order) ::: dom/base/ResizeObserver.h:107 (Diff revision 3) > + LinkedList<ResizeObservation> mObservationList; > +}; > + > +/** > + * ResizeObserverEntry is the entry that contains the information for observed > + * elements. This object is the one that visible to JavaScript in callback Two language nits: s/that visible/that's visible/ (or "that is visible") s/in callback/in the callback/ ::: dom/base/ResizeObserver.cpp:7 (Diff revision 3) > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/dom/ResizeObserver.h" You need to #include some more stuff here. Right now, this is relying on unified builds to provide all of its #includes (from other .cpp files that it gets grouped with) -- but that's fragile. To catch those issues (so you can fix them), temporarily put this file in SOURCES instead of UNIFIED_SOURCES in your moz.build, and then recompile. That'll give you some build errors, like e.g. this one: > ResizeObserver.cpp:129:9: error: use of undeclared identifier 'nsContentUtils'; (That's a hint that you need to include nsContentUtils.h here.) Please fix all such issues that you catch locally, and then you can revert the moz.build tweak (putting this back into UNIFIED_SOURCES). ::: dom/base/ResizeObserver.cpp:108 (Diff revision 3) > + > + MOZ_ASSERT(!mObservationList.isEmpty(), > + "If ResizeObservation found for an element, observation list " > + "must be not empty."); > + > + observation->remove(); Please move this up to be *just before* we call mObservationMap.Remove(). Broadly, I'd like to maintain the following invariant: mObservationMap should be considered the "owning" storage for the observations, and we should never let sometihng exist in the LinkedList unless it *also* exists in mObservationMap. (because the LinkedList doesn't have owning pointers to any of its entries, so it should lean on mObservationMap for providing that guarantee-of-them-being-still-alive) In this case you've got a local RefPtr so there's no risk. But still, let's enforce that invariant for extra sanity's sake. Please also audit the other places where we touch these objects, too, to be sure we add/remove to them in such a way as to preserve that invariant. (so: when inserting, insert into the hash table first; when clearing/removing, clear/remove from the linkedlist first) ::: dom/base/ResizeObserver.cpp:266 (Diff revision 3) > + nsRect rect = GetTargetRect(); > + return (rect.width != mBroadcastWidth || rect.height != mBroadcastHeight); > +} > + > +void > +ResizeObservation::UpdateBroadcastSize(nsRect aRect) Since this method has "size" in the name & only cares about size (not x/y), it should take "nsSize aSize". (and use aSize.width and aSize.height, instead of aRect.width and aRect.height) And its caller should pass in |rect.Size()| instead of |rect|.
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review77676 r=me -- let's have smaug take a look at this now. ::: dom/base/ResizeObserverController.h:101 (Diff revision 5) > + void GatherAllActiveObservations(uint32_t aDepth); > + > + /* > + * Calls BroadcastActiveObservations() for all ResizeObservers in this > + * controller. It also returns the shallowest depth of observed target > + * elements from all ResizeObserver or UINT32_MAX if there is no any This part isn't quite true: "returns the shallowest depth of observed target elements" But, I think it will become true if you add... "with active observations" ...at the end there (after "elements"). ::: dom/base/ResizeObserverController.h:102 (Diff revision 5) > + > + /* > + * Calls BroadcastActiveObservations() for all ResizeObservers in this > + * controller. It also returns the shallowest depth of observed target > + * elements from all ResizeObserver or UINT32_MAX if there is no any > + * active obsevations at all. A few small typos: s/is no any/aren't any/ s/obsevations/observations/ (missing an "r") ::: dom/base/ResizeObserverController.cpp:7 (Diff revision 5) > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/dom/ResizeObserverController.h" I suspect you might need some more #includes here, too -- be sure to give this the same (temporary, local) s/UNIFIED_SOURCES/SOURCES/ treatment that I suggested for the other patch above, to flush out any include-dragons that are hiding here. ::: dom/base/ResizeObserverController.cpp:132 (Diff revision 5) > + // that have the depth of observed target element more than current > + // shallowestTargetDepth. > + GatherAllActiveObservations(shallowestTargetDepth); > + } > + > + mResizeObserverNotificationHelper->Unregister(); I think mResizeObserverNotificationHelper is the only caller of this function at this point. So maybe it'd be simpler to get rid of this "Unregister()" call here, and just have the ResizeObserverNotificationHelper directly unregister itself after we've returned from this function? (at the end of ResizeObserverNotificationHelper's WillRefresh() method)
Attachment #8789818 - Flags: review?(dholbert) → review+
Comment on attachment 8789906 [details] Bug 1272409 part 4: Integrate ResizeObserver with Document and reflow. https://reviewboard.mozilla.org/r/77954/#review77678 r=me; again, smaug should take a final look.
Attachment #8789906 - Flags: review?(dholbert) → review+
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review77676 > I think mResizeObserverNotificationHelper is the only caller of this function at this point. So maybe it'd be simpler to get rid of this "Unregister()" call here, and just have the ResizeObserverNotificationHelper directly unregister itself after we've returned from this function? (at the end of ResizeObserverNotificationHelper's WillRefresh() method) We still need Unregister() method because we will call ScheduleNotification() (which will call mResizeObserverNotificationHelper->Register()) in here: if (HasAnySkippedObservations()) { .. // We need to deliver pending notifications in next cycle. ScheduleNotification(); } If we unregister after mOwner->Notify() in ResizeObserverNotificationHelper's WillRefresh(), ScheduleNotification() will take no effect (because ResizeObserverNotificationHelper still registered in RefreshDriver).
Comment on attachment 8789818 [details] Bug 1272409 part 3: Add ResizeObserverController. https://reviewboard.mozilla.org/r/77898/#review77676 > We still need Unregister() method because we will call ScheduleNotification() (which will call mResizeObserverNotificationHelper->Register()) in here: > > if (HasAnySkippedObservations()) { > .. > > // We need to deliver pending notifications in next cycle. > ScheduleNotification(); > } > > If we unregister after mOwner->Notify() in ResizeObserverNotificationHelper's WillRefresh(), ScheduleNotification() will take no effect (because ResizeObserverNotificationHelper still registered in RefreshDriver). Ah, thanks for clarifying that. So in that case, instead of "unregister ... maybe handle errors & re-register", let's rewrite this logic as just "maybe unregister, otherwise handle errors". I think it should look like: if (!HasSkippedNotifications()) { Unregister(); } else { // Per spec, we deliver an error if the document has any skipped observations. // Also, we stay registered so that we can eventually deliver // those observations. (Though, this doesn't match the spec -- see github issue [...bunch of code here but nothing to do with registering/scheduling...] }
Comment on attachment 8789907 [details] Bug 1272409 part 5: Add web-platform-tests for ResizeObserver. https://reviewboard.mozilla.org/r/77956/#review77870 Please split this tests patch into two changesets: * The first changeset should just be a straight import of the unmodified test files from upstream (though with your file-renaming and with your "Source" header comments -- but with no other changes at all). It should have "Import web-platform-tests..." instead of "Add web-platform-tests" in its commit message. * The second changeset can contain any tweaks that we need to make. (And ideally, please include code-comments alongside each change, explaining why we need the different behavior, and file a github issue if appropriate). (I just filed https://github.com/WICG/ResizeObserver/issues/23 for the <img> one quirk -- you'll want to link to that in ResizeObserver-observe.html.) The MANIFEST.json changes (and imghelper.png) should probably go in the *second* changeset, too -- so that the first changeset imports the files, but we won't actually run the tests until the changeset that fixes them up. This separation will be nice because it'll make it easier to see what our changes are, and it'll also make it easier to import (inevitable) test-changes from upstream. (Any extra tests that you're adding here (for edge cases not covered by the upstream tests) could either go in the second changeset, or in a third "Add entirely new tests" changeset.)
Attachment #8789907 - Flags: review?(dholbert) → review-
Comment on attachment 8789907 [details] Bug 1272409 part 5: Add web-platform-tests for ResizeObserver. https://reviewboard.mozilla.org/r/77956/#review77870 Let's avoid adding "imghelper.png", too -- I think it's cleaner to just use an inline data URL for a trivial SVG image document. See my proposed change to the <img> test in the github issue here: https://github.com/WICG/ResizeObserver/pull/24/commits/77ef49d2cd8adfafaed219fc4b96c0d4e1441280 That change might already be merged into the upstream test by the time you regenerate these patches, but if it's not, I'd suggest that you make that change as one of our "local tweaks" (in the second half of the split-up tests patch), and get rid of imghelper.png entirely.
Comment on attachment 8789907 [details] Bug 1272409 part 5: Add web-platform-tests for ResizeObserver. https://reviewboard.mozilla.org/r/77956/#review77870 And actually, in the interests of being able to stomp on these tests with updated versions when updates are available: - Don't bother adding a "Source:" comment at the top. (Instead, just link to the source in the commit message for the patch that imports these tests.) - Don't bother renaming the tests. This will make it easier to reimport the tests down the line when they're updated. (We'll be able to just pull in the updated files, and reapply your tweaks patch, to the extent that it still applies.) BTW -- I did some brief searching, to be 100% sure we're OK to pull these in without attribution, and we are -- since these are hosted in a w3c test suite, they should be covered by https://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html which says that tests are available under a 3-clause BSD license (which allows us to copy/alter without permission as long as we don't use any w3c logos). :)
Comment on attachment 8789904 [details] Bug 1272409 part 1: Add GetNodeDepth() to nsContentUtils. https://reviewboard.mozilla.org/r/77950/#review77932 ::: dom/base/nsContentUtils.cpp:9563 (Diff revision 3) > +{ > + uint32_t depth = 1; > + > + MOZ_ASSERT(aNode, "Node shouldn't be null"); > + > + while ((aNode = aNode->GetParentNode())) { Curious, what should happen in Shadow DOM case? This crosses anonymous (XBL) and native anonymous boundary, but not shadow boundary. I assume that is fine for now at least.
Attachment #8789904 - Flags: review?(bugs) → review+
Is this going to land soon? See bug 1109868. It would really help to unblock a privacy/security feature we would like to land in 53. Thank you.
Flags: needinfo?(farislab)
(In reply to Erin Lancaster [:elan] from comment #59) > Is this going to land soon? This has preliminary review feedback (from me) that needs addressing before it gets final review (from smaug, who knows our event-dispatching code better than I do & will hence provide a "real" review). Once that's all happened, this will be ready to land. Fariskhi's back in school now, so I'm not sure if he has cycles to push this over the line. I could hopefully take on the remaining work here in the next week or two. (Fariskhi, please chime in if you're planning on circling back to this soon -- if not, I'll jump in.) > See bug 1109868. It would really help to unblock a privacy/security feature we would like to land in 53. Thank you. It's not guaranteed that this will be ready to ride the trains in the same release where it lands on trunk. But we can hope (& might be able to expose it to UI without letting it ship for web content, if really needed).
Flags: needinfo?(dholbert)
Two notes: 1. I reached out to Farshiki in July on Twitter and he will not be able to return to this. 2. Best I can tell, Chrome will be shipping ResizeObserver in October https://twitter.com/atotic/status/894642766218575872
Based on comment 61.
Assignee: farislab → nobody
Status: ASSIGNED → NEW
There's an Intent to Ship for this on blink-dev: https://groups.google.com/a/chromium.org/d/msg/blink-dev/z6ienONUb5A/GlHRa9S2AAAJ It looks like the implementation work in Gecko has stopped, but we are interpreting the initial activity as a positive signal of sorts. Please shout if shipping this now is not a good idea.
This has now landed in Chrome 64: https://bugs.chromium.org/p/chromium/issues/detail?id=612962 Would love to see this in Firefox, please :-).
On Firefox 57 on Linux version this breaks some polyfills so I can't use them e.g. https://que-etc.github.io/resize-observer-polyfill/). Will this be implemented soon?
(In reply to mtrebizan from comment #65) > On Firefox 57 on Linux version this breaks some polyfills so I can't use > them e.g. https://que-etc.github.io/resize-observer-polyfill/). This sentence does't make sense to me. The example you linked is a polyfill *for* this feature. The polyfill may or may not be broken (not sure) -- but if it is broken, it's not broken by this bug (which covers the feature that the polyfill is filling in for). > Will this be implemented soon? Hopefully, but unclear; this needs some developer resources to unbitrot & finalize the patches here & push this over the finish line, and we've been quite busy focusing on other priorities. I'm hoping we can have someone get to this within the next couple months, though.
no I just thought that perhaps ResizeObservable was already implemented and that it somehow confused this polyfill not to activate. thanks for the answer!
Any updates on this?
It's on the radar for Q1 IIRC.

As a result of developer feedback, there is work being done
on extending current ResizeObserver API.

The main change is that instead of always observing content-box, now you can also
observe more boxes:
border-box
scroll-box: for chat windows
device-pixel-border-box: for <canvas> developers, allows for HiDpi canvas without moire patterns.

https://drafts.csswg.org/resize-observer-1/

The rest of the API looks the same. We'd love to hear any thoughts you have.

We are planning on soliciting feedback in CSSWG February F2F.

I have a local working patch, no tests yet. If you are interested in developing
to the newer spec, I can prioritize creating tests.

These are great additions. But even having the "old" spec would be a huge help.

Whiteboard: [layout:backlog:2019q2]

Safari/Webkit has now finished implementation of this feature:

https://bugs.webkit.org/show_bug.cgi?id=157743

With Edge moving to the Chromium codebase this year, Firefox will be the odd man out of the popular browsers here ;-).

As doa says above, even having support for the 'old' spec would be most helpful!

Thanks!

Whiteboard: [layout:backlog:2019q2] → [layout:backlog:2019q2:69]
Assignee: nobody → boris.chiou

I'm looking at these old patches and trying to apply them. May update them based on the current spec [1].

Cancel the ni? for farislab and dholbert.

[1] https://drafts.csswg.org/resize-observer/

Flags: needinfo?(farislab)
Flags: needinfo?(dholbert)
Alias: resize-observer → resize-observer-v1
Type: defect → enhancement
Blocks: 1543839

When rebasing these patches, I just noticed that part 5, Add wpt for ResizeObserver, is in the tree already. Thanks, Aleksandar. However, I saw some test failures based on the rebased patches. Still looking into them.

GetNodeDepth() will be used in the next patch.

Depends on D27614

This implements the first version of spec, so the webidl file doesn't
match the current spec and we will fix them in the follow-up bugs.

i.e.

  1. The default observer box is content-box.
  2. ResizeObserverBoxOptions, ResizeObserverOptions, and ResizeObserverSize
    are not included in ResizeObserver.webidl.
  3. ResizeObserverEntry doesn't have borderBoxSize and contentBoxSize
    attributes.

Depends on D27615

Use ResizeObserverController to schedule the observers and manage them.
Document will hold this controller in the later patch.

Depends on D27616

Depends on D27618

Attachment #8789818 - Attachment is obsolete: true
Attachment #8789904 - Attachment is obsolete: true
Attachment #8789905 - Attachment is obsolete: true
Attachment #8789906 - Attachment is obsolete: true
Attachment #8789907 - Attachment is obsolete: true
Attachment #9058439 - Attachment is obsolete: true
Attachment #9058440 - Attachment description: Bug 1272409 - Part 2: Add GetNodeDepth() to nsContentUtils. → Bug 1272409 - Part 1: Add GetNodeDepth() to nsContentUtils.
Attachment #9058441 - Attachment description: Bug 1272409 - Part 3: Add ResizeObserver webidl and implementation. → Bug 1272409 - Part 2: Add ResizeObserver webidl and implementation.
Attachment #9058442 - Attachment description: Bug 1272409 - Part 4: Add ResizeObserverController. → Bug 1272409 - Part 3: Add ResizeObserverController.
Attachment #9058443 - Attachment description: Bug 1272409 - Part 5: Integrate ResizeObserver with Document and reflow. → Bug 1272409 - Part 4: Integrate ResizeObserver with Document and reflow.
Attachment #9058444 - Attachment description: Bug 1272409 - Part 6: Update test meta. → Bug 1272409 - Part 5: Update test meta.
Blocks: 1545239
Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d817fbfc52b6 Part 1: Add GetNodeDepth() to nsContentUtils. r=smaug https://hg.mozilla.org/integration/autoland/rev/c331a4a8b343 Part 2: Add ResizeObserver webidl and implementation. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/72775dbf35c8 Part 3: Add ResizeObserverController. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/224dad4cbdc3 Part 4: Integrate ResizeObserver with Document and reflow. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/2ad8260489d6 Part 5: Update test meta. r=dholbert

Backed out 5 changesets (Bug 1272409) for build bustage at /builds/worker/workspace/build/src/dom/base/nsGlobalWindowCommands. On a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=2ad8260489d690f7615abb7fe27890bdb430de9a

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=242741263&repo=autoland&lineNumber=17277

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=3791fc50da340ff41e78eedf892454b395e47617

[task 2019-04-25T23:31:00.235Z] 23:31:00 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/bindings/test'
[task 2019-04-25T23:31:04.140Z] 23:31:04 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -isysroot /builds/worker/workspace/build/src/MacOSX10.11.sdk --target=x86_64-darwin11 -o Unified_cpp_dom_base8.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DDEBUG=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/netwerk/url-classifier -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fcrash-diagnostics-dir=/builds/worker/artifacts -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fsanitize=fuzzer-no-link -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base8.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp:2:
[task 2019-04-25T23:31:04.163Z] 23:31:04 ERROR - /builds/worker/workspace/build/src/dom/base/nsGlobalWindowCommands.cpp:487:10: error: unknown type name 'Document'; did you mean 'imgRequestProxyStatic::Document'?
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - RefPtr<Document> doc = window->GetExtantDoc();
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - ^~~~~~~~
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - imgRequestProxyStatic::Document
[task 2019-04-25T23:31:04.163Z] 23:31:04 INFO - /builds/worker/workspace/build/src/image/imgRequestProxy.h:55:34: note: 'imgRequestProxyStatic::Document' declared here
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - typedef mozilla::dom::Document Document;
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp:137:
[task 2019-04-25T23:31:04.164Z] 23:31:04 ERROR - /builds/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:214:11: error: reference to 'Animation' is ambiguous
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - const Animation* aAnimation) {
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/Layers.h:78:7: note: candidate found by name lookup is 'mozilla::layers::Animation'
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - class Animation;
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/KeyframeEffect.h:111:7: note: candidate found by name lookup is 'mozilla::dom::Animation'
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - class Animation;
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp:137:
[task 2019-04-25T23:31:04.164Z] 23:31:04 ERROR - /builds/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:213:46: error: out-of-line definition of 'GetTargetForAnimation' does not match any declaration in 'nsNodeUtils'
[task 2019-04-25T23:31:04.164Z] 23:31:04 INFO - Maybe<NonOwningAnimationTarget> nsNodeUtils::GetTargetForAnimation(
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - ^~~~~~~~~~~~~~~~~~~~~
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - /builds/worker/workspace/build/src/dom/base/nsNodeUtils.h:141:31: note: type of 1st parameter of member declaration does not match definition ('const mozilla::dom::Animation ' vs 'const mozilla::layers::Animation ')
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - GetTargetForAnimation(const mozilla::dom::Animation aAnimation);
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp:137:
[task 2019-04-25T23:31:04.165Z] 23:31:04 ERROR - /builds/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:215:39: error: member access into incomplete type 'const mozilla::layers::Animation'
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - AnimationEffect effect = aAnimation->GetEffect();
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/AnimationInfo.h:22:7: note: forward declaration of 'mozilla::layers::Animation'
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - class Animation;
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - ^
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base8.cpp:137:
[task 2019-04-25T23:31:04.165Z] 23:31:04 ERROR - /builds/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:222:36: error: reference to 'Animation' is ambiguous
[task 2019-04-25T23:31:04.165Z] 23:31:04 INFO - void nsNodeUtils::AnimationMutated(Animation* aAnimation,

hmm. Unified build failed. Too unlucky.

Attachment #9060872 - Attachment description: Bug 1272409 - Fix fuzzy unified build failed after adding file into dom/base. → Bug 1272409 - Fix fuzzy unified build failed after adding new files into dom/base.
Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7b8695f2ca82 Fix fuzzy unified build failed after adding new files into dom/base. r=dholbert https://hg.mozilla.org/integration/autoland/rev/2f3cfdbcb900 Part 1: Add GetNodeDepth() to nsContentUtils. r=smaug https://hg.mozilla.org/integration/autoland/rev/b634a26d0c2e Part 2: Add ResizeObserver webidl and implementation. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/0b7728e6fcda Part 3: Add ResizeObserverController. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/87c7a189cbe0 Part 4: Integrate ResizeObserver with Document and reflow. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/b86894409b1b Part 5: Update test meta. r=dholbert
Regressions: 1547485
Regressions: 1547533
Regressions: 1547567
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16580 for changes under testing/web-platform/tests
Whiteboard: [layout:backlog:2019q2:69] → [layout:backlog:2019q2:69][wptsync upstream]
Regressions: 1548057
Whiteboard: [layout:backlog:2019q2:69][wptsync upstream] → [layout:backlog:2019q2:69][wptsync upstream error]
Whiteboard: [layout:backlog:2019q2:69][wptsync upstream error] → [layout:backlog:2019q2:69][wptsync upstream]
Regressions: 1555786
Blocks: 1565557
Blocks: 1587973
Blocks: 1689099
Depends on: 1685038
Blocks: resize-observer-1
No longer depends on: 1685038
Alias: resize-observer-v1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: