Closed Bug 1365092 Opened 8 years ago Closed 8 years ago

Move side effects of SetAttr and ParseAttribute to BeforeSetAttr and AfterSetAttr

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions
59 bytes, text/x-review-board-request
Details
Bug 1365092 - Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions
59 bytes, text/x-review-board-request
Details
Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions
59 bytes, text/x-review-board-request
Details
Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
In order for attribute cloning to work (replacing the current mechanism of re-parsing), SetAttr and ParseAttr must not have effects that will not occur when cloning. These effects should be moved to BeforeSetAttr and AfterSetAttr.
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions This patch isn't ready yet, but I wanted to make sure that I got the changes to nsGenericHTMLElement right since the effects could be quite significant. Does this look ok to you?
Attachment #8867924 - Flags: feedback?(bzbarsky)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions Oop. I forgot the changes to ParseAttribute. I will add those and re-request.
Attachment #8867924 - Flags: feedback?(bzbarsky)
Attachment #8867924 - Flags: feedback?(bzbarsky)
Attachment #8867924 - Flags: feedback?(bzbarsky)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions Looks like there are some assertions I need to take care of.
Attachment #8867924 - Flags: feedback?(bzbarsky)
Attachment #8867924 - Flags: feedback?(bzbarsky)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review143616 ::: dom/base/Element.h:1422 (Diff revision 4) > * @param aValue the value it's being set to represented as either a string or > * a parsed nsAttrValue. Alternatively, if the attr is being removed it > * will be null. > * @param aNotify Whether we plan to notify document observers. > */ > // Note that this is inlined so that when subclasses call it it gets It's not inlined anymore. ::: dom/base/Element.h:1443 (Diff revision 4) > * @param aOldValue the value that the attribute was previously. If null, > * the attr was not previously set. This attribute may not currently > * be set properly for SVG Elements. > * @param aNotify Whether we plan to notify document observers. > */ > // Note that this is inlined so that when subclasses call it it gets This is not inlined any more either. ::: dom/base/Element.cpp (Diff revision 4) > return true; > } > if (aAttribute == nsGkAtoms::id) { > // Store id as an atom. id="" means that the element has no id, > // not that it has an emptystring as the id. > - RemoveFromIdTable(); A few notes here: 1) Now that the mutating bits here are gone, I'm pretty sure that the "Even the value was pre-parsed, we still need to call ParseAttribute" comment in Element::SetAttr is no longer true, and we should in fact not do that ParseAttribute call in the pre-parsed case. And perhaps assert that we don't get ParseAttribute for the class attr? 2) Still in Element::SetAttr, there is a scriptblocker that gets put in place before ParseAttribute (in the form of mozAutoDocUpdate), because ParseAttribute can notify id observers. Now that we're going to notify those id observers from BeforeSetAttr, we should move that mozAutoDocUpdate and its comment to before the BeforeSetAttr call. ::: dom/base/Element.cpp:2614 (Diff revision 4) > +Element::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValueOrString* aValue, bool aNotify) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::_class) { > + SetMayHaveClass(); We probably shouldn't SetMayHaveClass() if !aValue. Ideally we'd ClearMayHaveClass but see below (in the id flag discussion) for some complications. ::: dom/base/Element.cpp:2616 (Diff revision 4) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::_class) { > + SetMayHaveClass(); > + } > + if (aName == nsGkAtoms::id) { else if? ::: dom/base/Element.cpp:2620 (Diff revision 4) > + if (aValue && !aValue->IsEmpty()) { > + SetHasID(); > + } else { > + ClearHasID(); > + } This seems unsound because it will ClearHasID if the value even though the value hasn't actually changed yet. This didn't matter before, because we didn't use to run any subclass code between this call and the value changing to match. But now we are running this whenever a subclass happens to call BeforeSetAttr, which might be before it triggers something that causes a state change. And then that state change might not cause restyling properly because it will think we have no id in the "before the state changed" state, when we actually do. I think what we may want instead is to run code right _after_ calling BeforeSetAttr which does the RemoveFromIdTable bit, and more code right _before_ calling AfterSetAttr which readds to the id table and sets/clears the id and class flags. Maybe create helper functions for both those things instead of duplicating the code in SetAttr/SetParsedAttr/SetAttrAndNotify and UnsetAttr. ::: dom/html/nsGenericHTMLElement.cpp:653 (Diff revision 4) > + const nsAttrValueOrString* aValue, > + bool aNotify) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::contenteditable) { > + SetMayHaveContentEditableAttr(); I guess this is behavior-preserving, generally, but conceptually this should perhaps be in AfterSetAttr. Maybe file a followup to do that, or document why it's here if there's a good reason. ::: dom/html/nsGenericHTMLElement.cpp:655 (Diff revision 4) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::contenteditable) { > + SetMayHaveContentEditableAttr(); > + } else if (aName == nsGkAtoms::accesskey) { > + UnregAccessKey(); Maybe keep the comment about how this has to happen before we clear the flag? ::: dom/html/nsGenericHTMLElement.cpp:660 (Diff revision 4) > + UnregAccessKey(); > + if (!aValue) { > + UnsetFlags(NODE_HAS_ACCESSKEY); > + } > + } else if (aName == nsGkAtoms::name) { > + RemoveFromNameTable(); We probably want to keep the comment about how this needs to happen before the ClearHasName call. ::: dom/html/nsGenericHTMLElement.cpp:718 (Diff revision 4) > } > } > SetDirectionalityOnDescendants(this, dir, aNotify); > + } else if (aName == nsGkAtoms::contenteditable) { > + int32_t editableCountDelta = 0; > + if (aOldValue && (aOldValue->Equals(nsGkAtoms::_true, eIgnoreCase) || If eIgnoreCase, then the nsIAtom\* version of nsAttrValue::Equals has to do strictly more work than then nsAString version. So just use NS_LITERAL_STRING("true") and EmptyString() in these checks. ::: dom/html/nsGenericHTMLElement.cpp:722 (Diff revision 4) > + int32_t editableCountDelta = 0; > + if (aOldValue && (aOldValue->Equals(nsGkAtoms::_true, eIgnoreCase) || > + aOldValue->Equals(nsGkAtoms::_empty, eIgnoreCase))) { > + editableCountDelta = -1; > + } > + if (aValue && (aValue->Equals(nsGkAtoms::_true, eIgnoreCase) || And here, strings, not atoms. ::: dom/html/nsGenericHTMLElement.cpp:728 (Diff revision 4) > + aValue->Equals(nsGkAtoms::_empty, eIgnoreCase))) { > + ++editableCountDelta; > + } > + ChangeEditableState(editableCountDelta); > + } else if (aName == nsGkAtoms::accesskey) { > + if (aValue && !aValue->Equals(nsGkAtoms::_empty, eIgnoreCase)) { And here, string. ::: dom/html/nsGenericHTMLElement.cpp:733 (Diff revision 4) > + if (aValue && !aValue->Equals(nsGkAtoms::_empty, eIgnoreCase)) { > + SetFlags(NODE_HAS_ACCESSKEY); > + RegAccessKey(); > + } > + } else if (aName == nsGkAtoms::name) { > + if (aValue && !aValue->Equals(nsGkAtoms::_empty, eIgnoreCase) && Same thing here wrt strings.
Attachment #8867924 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review143616 Thank you for your review and all of your help! I have finally figured out how to post more than one reviewboard patch per bug (turns out it's really easy), so after I fix these issues I will request a review on this and add more patches when I have that code ready rather than adding code to this patch. > If eIgnoreCase, then the nsIAtom\* version of nsAttrValue::Equals has to do strictly more work than then nsAString version. So just use NS_LITERAL_STRING("true") and EmptyString() in these checks. Hmm. You seem to be correct. I actually thought I was avoiding that case, but I seem to have misread the `nsAttrValue::Equals` implementation. In the case where `aName == nsGkAtoms::accesskey`, I agree that a string comparison should be used because I *think* that the attribute value will also be a string. However, when `aName == nsGkAtoms::contenteditable` and when `aName == nsGkAtoms::name`, I believe that the value is stored as an atom [1]. Therefore, a string comparison seems silly when we have the atom comparison available. So what about this idea: What if I implement a function called something like `nsAttrValue::IsAtom` that takes only one argument, an atom, and asserts that `this` value that is also an atom before making an atom-to-atom comparison? If not, I can convert these lines to string comparisons. [1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/dom/html/nsGenericHTMLElement.cpp#928,939
Flags: needinfo?(bzbarsky)
> Therefore, a string comparison seems silly when we have the atom comparison available. Not in the eIgnoreCase case. The nsIAtom overload of nsAttrValue::Equals will always ToString the incoming atom and then call the string version if aCaseSensitive != eCaseMatters. You could probably do better with an nsDependentAtomString, but that's about it; we don't really have case-insensitive compare functions for atoms...
Flags: needinfo?(bzbarsky)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review143616 > A few notes here: > > 1) Now that the mutating bits here are gone, I'm pretty sure that the "Even the value was pre-parsed, we still need to call ParseAttribute" comment in Element::SetAttr is no longer true, and we should in fact not do that ParseAttribute call in the pre-parsed case. And perhaps assert that we don't get ParseAttribute for the class attr? > > 2) Still in Element::SetAttr, there is a scriptblocker that gets put in place before ParseAttribute (in the form of mozAutoDocUpdate), because ParseAttribute can notify id observers. Now that we're going to notify those id observers from BeforeSetAttr, we should move that mozAutoDocUpdate and its comment to before the BeforeSetAttr call. I will change the `ParseAttribute` class attribute check to an assertion in the patch that actually ensures that those attributes are set via `SetParsedAttribute`. This patch, however, only moves side effects from `SetAttr` and `ParseAttribute` to `BeforeSetAttr` and `AfterSetAttr` so this assertion will currently fail if added.
This blocks a qf:p1 bug so I'm also marking it as qf:p1.
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review143616 > I will change the `ParseAttribute` class attribute check to an assertion in the patch that actually ensures that those attributes are set via `SetParsedAttribute`. This patch, however, only moves side effects from `SetAttr` and `ParseAttribute` to `BeforeSetAttr` and `AfterSetAttr` so this assertion will currently fail if added. Nevermind, I will change this now.
Comment on attachment 8871009 [details] Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142572/#review146182 ::: dom/html/HTMLImageElement.cpp:581 (Diff revision 1) > > - return rv; > + return NS_OK; > +} > + > +nsresult > +HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) Uh. I have no idea why Review Board did the diff like this. I think maybe it overly aggressive attempts to say that a line changed rather than removed caused this? My patch makes *NO* changes to `HTMLImageElement::GetEventTargetParent`, so maybe just ignore this...
Comment on attachment 8871009 [details] Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142572/#review146192 ::: dom/html/HTMLImageElement.cpp:581 (Diff revision 1) > > - return rv; > + return NS_OK; > +} > + > +nsresult > +HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) Ah. I see why it is. *I* consider my patch to have deleted a function and moved the code that was in it to two new functions in a different place. But *Review Board* sees it as a function being renamed and split, and the "in between" functions being moved around it.
Thanks for working with me on this. The first three patches are ready for review. I am still fixing some problems with the last patch.
Flags: needinfo?(bzbarsky)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review148170 r=me with the comments below. ::: dom/base/Element.h:1503 (Diff revision 7) > + * > + * @param aNamespaceID the namespace of the attr being set > + * @param aName the localname of the attribute being set > + * @param aValue the new id value. Will be null if the id is being unset. > + */ > + nsresult PreIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, This function always returns NS_OK. Why not must make it return void instead? ::: dom/base/Element.h:1516 (Diff revision 7) > + * > + * @param aNamespaceID the namespace of the attr being set > + * @param aName the localname of the attribute being set > + * @param aValue the new id value. Will be null if the id is being unset. > + */ > + nsresult PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, This, too, should return void. ::: dom/base/Element.cpp:2633 (Diff revision 7) > nsIAtom* aAttribute, > const nsAString& aValue, > nsAttrValue& aResult) > { > if (aNamespaceID == kNameSpaceID_None) { > - if (aAttribute == nsGkAtoms::_class) { > + NS_ASSERTION(aAttribute != nsGkAtoms::_class, MOZ_ASSERT, please. ::: dom/base/Element.cpp:2667 (Diff revision 7) > + const nsAttrValueOrString* aValue, bool aNotify) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::_class) { > + if (aValue) { > + SetMayHaveClass(); This should probably document the asymmetry with the id flag. Specifically, this flag is never unset and isn't exact (unlike the id flag), so we're OK as long as we set it at some point before the class attr ever gets set. If we ever try to make it exact, we'll need to do something similar to what we do for ids. ::: dom/base/Element.cpp:2680 (Diff revision 7) > +nsresult > +Element::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValue* aValue, const nsAttrValue* aOldValue, > + bool aNotify) > +{ > + return NS_OK; This could stay inline in the header, given that it does nothing, right? And keep the comment about how it's inline there on purpose, for performance. ::: dom/base/Element.cpp:2694 (Diff revision 7) > + if (aValue && !aValue->IsEmpty()) { > + SetHasID(); > + } else { > + ClearHasID(); It's worth documenting why this code to SetHasID/ClearHasID is not in PostIdMaybeChange. And if we don't have a reason, perhaps it should move to there... ::: dom/html/nsGenericHTMLElement.h (Diff revision 7) > nsIContent* aBindingParent, > bool aCompileEventHandlers) override; > virtual void UnbindFromTree(bool aDeep = true, > bool aNullParent = true) override; > > - MOZ_ALWAYS_INLINE // Avoid a crashy hook from Avast 10 Beta It's worth checking why that MOZ_ALWAYS_INLINE was added and what, if anything, its replacement should be. ::: dom/html/nsGenericHTMLElement.cpp:717 (Diff revision 7) > dir = RecomputeDirectionality(this, aNotify); > } > } > SetDirectionalityOnDescendants(this, dir, aNotify); > + } else if (aName == nsGkAtoms::contenteditable) { > + SetMayHaveContentEditableAttr(); So I thought about this some more, and I realized what the reason is to have this early. Since its also not an exact flag, and since we may have subclass code run before our AfterSetAttr, we should really set this in BeforeSetAttr, but only when aValue is not null. That way subclasses won't get think we're missing a contenteditable attr when we actually have one. Please move this back to BeforeSetAttr but document why it's there. This only applies to the SetMayHaveContentEditableAttr() call. ::: dom/html/nsGenericHTMLElement.cpp:738 (Diff revision 7) > + RegAccessKey(); > + } > + } else if (aName == nsGkAtoms::name) { > + if (aValue && !aValue->Equals(EmptyString(), eIgnoreCase) && > + CanHaveName(NodeInfo()->NameAtom())) { > + SetHasName(); This is a bit messed up in that we can have subclass code run before here. But I suspect in practice subclasses don't care about this flag, and in particular selector matching does not care. Otherwise we'd want to handle it like we handle id attributes.... Worth documenting what's going on here.
Attachment #8867924 - Flags: review+
Comment on attachment 8870937 [details] Bug 1365092 - Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142498/#review148182 r=me with the logic simplifications below, though I welcome evidence that they are not valid simplifications. ::: dom/html/HTMLAnchorElement.cpp:389 (Diff revision 1) > - // However, if we have a cached URI, we'll want to see if the value changed. > + // However, if we have a cached URI, we'll want to see if the value > + // changed. > - else { > + else { > - nsAutoString val; > - GetHref(val); > - if (!val.Equals(aValue)) { > + const nsAttrValue* oldValue = > + mAttrsAndChildren.GetAttr(nsGkAtoms::href); > + if (!aValue->EqualsAsStrings(*oldValue)) { You shouldn't ever end up here if the two values are equal as strings. We optimize that out quite early in SetAttr. The old code made sense when we were basically going from a relative to an absolute URL that was the resolved version of the relative URL (i.e. when someone did `a.href = a.href`). The new code doesn't really make sense to me. I suppose we could compare the new value to the serialization of our cached URI. But that involves annoying encoding conversions, if nothing else. So maybe it's simpler to just always have "reset = true" here, effectively. Might be worth a try push to see whether "reset" ever ends up being false; I suspect it does not. ::: dom/html/HTMLAnchorElement.cpp:413 (Diff revision 1) > - // that content states have changed will call IntrinsicState, which will try > - // to get updated information about the visitedness from Link. > - if (reset) { > + const nsAttrValue* aOldValue, bool aNotify) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::href) { > + if (aValue) { > + if (!Link::HasCachedURI() || !aOldValue || Here, again, aOldValue and aValue should never end up equal. So this test is always true as far as I can tell.
Attachment #8870937 - Flags: review+
Comment on attachment 8871009 [details] Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142572/#review148184 r=me, but please make sure the followup bug I'm asking for below gets filed. ::: dom/base/Element.h:1532 (Diff revision 2) > + * @param aName the localname of the attribute being set > + * @param aValue the value it's being set to represented as either a string or > + * a parsed nsAttrValue. > + * @param aNotify Whether we plan to notify document observers. > + */ > + virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName, Does this need to return nsresult? Our one use case doesn't need to. ::: dom/base/Element.h:1536 (Diff revision 2) > + */ > + virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValueOrString& aValue, > + bool aNotify) > + { > + return NS_OK; Worth a comment about why this is inlined, like we have for AfterSetAttr. ::: dom/html/HTMLImageElement.h:373 (Diff revision 2) > + * @param aName the localname of the attribute being set > + * @param aValue the value it's being set to represented as either a string or > + * a parsed nsAttrValue. > + * @param aNotify Whether we plan to notify document observers. > + */ > + nsresult BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, This can return void. ::: dom/html/HTMLImageElement.h:384 (Diff revision 2) > + * > + * @param aNamespaceID the namespace of the attr being set > + * @param aName the localname of the attribute being set > + * @param aNotify Whether we plan to notify document observers. > + */ > + nsresult AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, This can return void. ::: dom/html/HTMLImageElement.h:391 (Diff revision 2) > + /** > + * Used by BeforeMaybeChangeAttr and AfterMaybeChangeAttr to keep track of > + * whether a reload needs to be forced after an attribute change that is > + * currently in progress. > + */ > + bool mForceReload; This is rather annoying, but I don't have a better idea. ::: dom/html/HTMLImageElement.cpp:486 (Diff revision 2) > nsresult > -HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName, > - nsIAtom* aPrefix, const nsAString& aValue, > +HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValueOrString& aValue, > - bool aNotify) > + bool aNotify) > { > - bool forceReload = false; > + mForceReload = false; I would rather we did this in AfterMaybeChangeAttr in the mForceReload case. That is, guarantee that AfterMaybeChangeAttr puts us back in our no-force-reload state. ::: dom/html/HTMLImageElement.cpp:497 (Diff revision 2) > // If we are in responsive mode, we drop the forced reload behavior, > // but still trigger a image load task for img.src = img.src per > // spec. > // > // Both cases handle unsetting src in AfterSetAttr > - if (aNameSpaceID == kNameSpaceID_None && > + if (aNamespaceID == kNameSpaceID_None && A bunch of this stuff could probably happen in the "After" function instead. Maybe a followup to do that? ::: dom/html/HTMLImageElement.cpp:534 (Diff revision 2) > - } else if (aNameSpaceID == kNameSpaceID_None && > + } else if (aNamespaceID == kNameSpaceID_None && > aName == nsGkAtoms::crossorigin && > aNotify) { > nsAttrValue attrValue; > - ParseCORSValue(aValue, attrValue); > + ParseCORSValue(aValue.String(), attrValue); > if (GetCORSMode() != AttrValueToCORSMode(&attrValue)) { I think we could do this in the "After" function if we just make sure to get our CORS modes to compare from the old value and from the new value, instead of from the current attr value and new value. ::: dom/html/HTMLImageElement.cpp:544 (Diff revision 2) > - aNameSpaceID == kNameSpaceID_None && > + aNamespaceID == kNameSpaceID_None && > aNotify) { > - ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue); > + ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue.String()); > if (!InResponsiveMode() && > referrerPolicy != RP_Unset && > referrerPolicy != GetImageReferrerPolicy()) { Similarly here, if we compared the old and new values. And then we wouldn't need the mForceReload boolean, since all this stuff would happen after the attr set. In fact, we wouldn't even need the "Before" function, I think. Again, followup is best for this; I agree that the patch for the moment preserves the current behavior and that's reasonable, risk-wise. ::: dom/html/HTMLImageElement.cpp:585 (Diff revision 2) > > - return rv; > + return NS_OK; > +} > + > +nsresult > +HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) Boy, did mozreview mess up this diff. :( The raw diff is a lot clearer....
Attachment #8871009 - Flags: review+
Is the fourth patch ready for review too, or still being worked on?
Flags: needinfo?(bzbarsky) → needinfo?(ksteuber)
The fourth patch isn't quite ready, but I am working on it.
Flags: needinfo?(ksteuber)
Comment on attachment 8867924 [details] Bug 1365092 - Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/139460/#review148170 > It's worth checking why that MOZ_ALWAYS_INLINE was added and what, if anything, its replacement should be. It appears to have been added to prevent Avast from hooking into that function in a buggy way (Bug 1058131). From the text of the bug, it appears that only nsGenericHTMLElement::SetAttr suffered from problems with this hook. After this patch, nsGenericHTMLElement::SetAttr will not exist directly, although a base class does have that method (Element::SetAttr). It seems like there would be no ill effects of inlining the base class method, so I guess I'll do that just to be safe.
Attachment #8870937 - Attachment is obsolete: true
Attachment #8871009 - Attachment is obsolete: true
Attachment #8871879 - Attachment is obsolete: true
Attachment #8870937 - Attachment is obsolete: false
Comment on attachment 8870937 [details] Bug 1365092 - Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142498/#review148182 > You shouldn't ever end up here if the two values are equal as strings. We optimize that out quite early in SetAttr. > > The old code made sense when we were basically going from a relative to an absolute URL that was the resolved version of the relative URL (i.e. when someone did `a.href = a.href`). The new code doesn't really make sense to me. > > I suppose we could compare the new value to the serialization of our cached URI. But that involves annoying encoding conversions, if nothing else. So maybe it's simpler to just always have "reset = true" here, effectively. > > Might be worth a try push to see whether "reset" ever ends up being false; I suspect it does not. I am not sure that we want to rely on the SetAttr optimization to be sure that the new value is not equal to the old value. See this comment [1]. I also do not understand why you say that the old code made sense. I *feel* like you are saying that the old comparison made since because it was a URL comparison rather than a string comparison (which I agree would make sense). But examining the code, it seems to just be a string comparison to me. Am I missing something? I think that the comparison should stay or be improved (to a URL comparison maybe?), but I would like to better understand your thinking here. [1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/base/Element.cpp#2331-2333
> I am not sure that we want to rely on the SetAttr optimization to be sure that the new value > is not equal to the old value. See this comment Ah, true. OK, so we could have the values differ in the specific case when aNotify is false. But that should only happen from the parsed in sane cases. So it won't be in the HasCachedURI() situation anyway. Anyone who's changing the href _after_ we have a cached URI and with aNotify false is really doing it wrong... And the cost of getting this wrong is that we do a little bit more work, in a case that really shouldn't happen. The cost of doing the Equals() check is doing extra work on normal href sets. > I *feel* like you are saying that the old comparison made since because it was a URL comparison The old code sort of made sense to optimize out things, maybe, when someone does "a.href = a.href" on an anchor; the old code compared the incoming string to the absolute URL GetHref returns and hence would avoid work in that case. But that was pretty much the only case in which it ended up doing anything useful. > Am I missing something? I think the main thing you might be missing is that GetHref() doesn't return the same thing as GetAttribute("href"). Improving to a URL comparison (in the sense of comparing the absolute URL before to the absolute URL after) might make sense if we want to avoid the DNSPrefetch/ResetLinkState bits at all costs, bug I suspect is more trouble than it's worth in practice. In an ideal world we'd get some data on how often the various comparisons here match or not, how much slowdown they add when they don't match, and how much win they are when they do match...
Comment on attachment 8871009 [details] Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/142572/#review148184 > Does this need to return nsresult? Our one use case doesn't need to. I would like to keep `OnAttrSetButNotChanged`'s return value as nsresult. Our current use case does not need it, but the next patch does use this.
Attachment #8870937 - Attachment is obsolete: true
The last patch is ready for review. Also btw, I screwed up my upload of the new versions of the other patches and lost your r+ on them. Sorry about that. Would you mind setting the flags back for me (unless, of course, there is a problem with them)?
Flags: needinfo?(bzbarsky)
Did anything change about the other patches other than addressing the review comments? mozreview is being totally unhelpful in terms of giving me interdiffs, probably for the same reasons that it lost the review flags...
I guess given the lack of interdiffs I just need to re-review from scratch. How I hate mozreview....
Yeah, I'm sorry about that. I've been struggling to understand how mozreview behaves with more than one patch, and I made a mistake.
OK, have this largely paged in again. For the first patch, we should document in comments why Pre/PostIdMaybeChange exist separately from Before/AfterSetAttr, because the need for them is pretty subtle.
Flags: needinfo?(bzbarsky)
And the comments on the SetMayHaveClass call I asked for in comment 31 still need to happen.
Flags: needinfo?(ksteuber)
Comment on attachment 8873638 [details] Bug 1365092 - Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/145028/#review150368 r=me with the nit below. ::: dom/html/HTMLAnchorElement.cpp:392 (Diff revision 2) > - CancelDNSPrefetch(HTML_ANCHOR_DNS_PREFETCH_DEFERRED, > - HTML_ANCHOR_DNS_PREFETCH_REQUESTED); > + if (IsInComposedDoc()) { > + TryDNSPrefetch(); Need to check for aValue too; we don't want to TryDNSPrefetch() if !aValue.
Attachment #8873638 - Flags: review+
And the followup bug for the various bits I asked for in comment 33 needs to happen.
Comment on attachment 8873639 [details] Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions https://reviewboard.mozilla.org/r/145030/#review150372 r=me, but please make sure the comment 33 followups happen! Ideally the bug numbers for them would be in the patch that lands here. ::: dom/base/Element.h:1540 (Diff revision 2) > + virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValueOrString& aValue, > + bool aNotify) Please fix the indent here.
Attachment #8873639 - Flags: review+
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review150376 This is awesome stuff. Not marking r+ yet because there are some substantive issues below, but none of them look like showstoppers; just fixups that need to happen. ::: dom/html/HTMLCanvasElement.cpp:492 (Diff revision 2) > + (aName == nsGkAtoms::width || aName == nsGkAtoms::height || > + aName == nsGkAtoms::moz_opaque)) { Weird indent here, please fix. ::: dom/html/HTMLCanvasElement.cpp:495 (Diff revision 2) > - { > +{ > + if (mCurrentContext && aNamespaceID == kNameSpaceID_None && > + (aName == nsGkAtoms::width || aName == nsGkAtoms::height || > + aName == nsGkAtoms::moz_opaque)) { > ErrorResult dummy; > - rv = UpdateContext(nullptr, JS::NullHandleValue, dummy); > + nsresult rv = UpdateContext(nullptr, JS::NullHandleValue, dummy); I don't think failures from UpdateContext here should be propagated out to the SetAttr() caller, and hence shouldn't be propagated out of this function. In particular, throwing an exception from setAttribute to script if we just run out of webgl contexts to allocate is pretty weird and not what the specs say to do at all, afaict. So we should not propagate this rv out of this function, and hence it can return void. In addition to that, none of the nsICanvasRenderingContextInternal::SetIsOpaque implementations return error. Please file a followup to change that function to return void? ::: dom/html/HTMLFormElement.cpp:197 (Diff revision 2) > -HTMLFormElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName, > - nsIAtom* aPrefix, const nsAString& aValue, > +HTMLFormElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValueOrString* aValue, bool aNotify) > - bool aNotify) > { > - if ((aName == nsGkAtoms::action || aName == nsGkAtoms::target) && > - aNameSpaceID == kNameSpaceID_None) { > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::action || aName == nsGkAtoms::target) { To be compatible with the old code, you want to check for aValue here, right? That is, we didn't flush pending submissions in UnsetAttr before... Might be worth adding a comment about how that's just preserving previous behavior. Not sure how to create a test for this... Maybe something that removes the relevant attributes from an onsubmit event? ::: dom/html/HTMLFrameSetElement.cpp:111 (Diff revision 2) > - rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aAttribute, aPrefix, > - aValue, aNotify); > +nsresult > +HTMLFrameSetElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName, > + const nsAttrValue* aValue, > + const nsAttrValue* aOldValue, bool aNotify) > +{ > mCurrentRowColHint = NS_STYLE_HINT_REFLOW; No, this doesn't do the same thing as the old code. I'd expect tests to fail as a result, if we had tests for this stuff... Unfortunately, bug 48422 predates us having a CI infrastructure or testing or any of that stuff. Do you mind adding some tests that would have caught that? Or I can do it if you prefer; let me know. Anyway, the point of this mCurrentRowColHint business is that it's used in HTMLFrameSetElement::GetAttributeChangeHint which is called from the restyle manager AttributeChanged implementations. Which means that it needs to be in the value the "before" hook set it to when we do the call to nsNodeUtils::AttributeChanged in Element::SetAttrAndNotify. But that happens after the AfterSetAttr call. Maybe what we should do is set mCurrentRowColHint to NS_STYLE_HINT_REFLOW at the beginning of BeforeSetAttr unconditionally (i.e. no matter what attr is changing). And document that the idea is that the only consumer is GetAttributeChangeHint, which is only called from AttributeChanged notifications, and this way we'll ensure that GetAttributeChangeHint sees NS_STYLE_HINT_REFLOW except in the cases in which our rows/cols are changing and it needs to see the ReconstructFrame hint. It's icky, but I don't think it's more icky than what we have here right now... ::: dom/html/HTMLIFrameElement.cpp:196 (Diff revision 2) > + bool aNotify) > +{ > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::srcdoc) { > + // Don't propagate errors from LoadSrc. The attribute was successfully > + // set, that's what we should reflect. "set or unset" ::: dom/html/HTMLIFrameElement.cpp:201 (Diff revision 2) > + // set, that's what we should reflect. > - LoadSrc(); > + LoadSrc(); > - } > + } > + } > > - return NS_OK; > + return; No need; it's a void function. Just fall off the end. ::: dom/html/HTMLLegendElement.cpp (Diff revision 2) > } > return retval; > } > > nsresult > -HTMLLegendElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute, Awesome dead code removal. ;) ::: dom/html/HTMLMediaElement.h:1719 (Diff revision 2) > uint32_t mCount; > }; > private: > + /** > + * This function is called by AfterSetAttr and OnAttrSetButNotChanged. > + * It will not be called if the value is being unset. This comment doesn't match implementation (i.e. AfterSetAttr is calling this function unconditionally). ::: dom/html/HTMLMediaElement.cpp:4198 (Diff revision 2) > nsresult > HTMLMediaElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName, > const nsAttrValue* aValue, > const nsAttrValue* aOldValue, bool aNotify) > { > - if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src) { > + AfterMaybeChangeAttr(aNameSpaceID, aName, aNotify); This doesn't look right to me. In particular, AfterMaybeChangeAttr just does the DoLoad() bits from SetAttr for the "src" attribute changing, right? Those bits used to happen _after_ the AfterSetAttr code for "src", but after this patch would happen _before_, right? And in particular before mSrcMediaSource is updated. I'm fairly surprised this does not cause any test failures... Might be worth asking one of the media folks to add a test it would have failed. Also, we didn't use to DoLoad() from UnsetAttr, so this call really does need to be conditional on aValue, afaict (and the header comments which don't match this code were correct). Not sure whether this is testabl, but if it is we should add a test. ::: dom/html/HTMLMediaElement.cpp:4258 (Diff revision 2) > + if (aName == nsGkAtoms::src) { > + DoLoad(); > + } > + } > + > + return; No need. ::: dom/html/HTMLSelectElement.cpp:1310 (Diff revision 2) > - aNameSpaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::disabled) { > + if (aNotify) { > - mDisabledChanged = true; > + mDisabledChanged = true; > - } > + } > + } else if (aName == nsGkAtoms::multiple) { > + if (!aValue && mSelectedIndex >= 0) { This used to be conditioned on aNotify. Please keep doing that? ::: dom/html/HTMLSelectElement.cpp:1343 (Diff revision 2) > mAutocompleteAttrState = nsContentUtils::eAutocompleteAttrState_Unknown; > mAutocompleteInfoState = nsContentUtils::eAutocompleteAttrState_Unknown; > + } else if (aName == nsGkAtoms::multiple) { > + // We might have become a combobox; make sure _something_ gets > + // selected in that case > + CheckSelectSomething(aNotify); This, too, used to be conditioned on aNotify. And should be conditioned on !aValue to keep the old behavior (in particular, if aValue, then we are definitely NOT a combobox and don't need to do anything here). ::: dom/html/nsGenericHTMLFrameElement.cpp:369 (Diff revision 2) > - } > + } else if (aName == nsGkAtoms::mozbrowser) { > - > - if (aName == nsGkAtoms::mozbrowser && aNameSpaceID == kNameSpaceID_None) { > - mReallyIsBrowser = !!aValue && BrowserFramesEnabled() && > + mReallyIsBrowser = !!aValue && BrowserFramesEnabled() && > - PrincipalAllowsBrowserFrame(NodePrincipal()); > + PrincipalAllowsBrowserFrame(NodePrincipal()); > + } else if (aName == nsGkAtoms::name) { This is subtly changing the behavior of the "name" attr. We used to propagate it to the docshell even if it were being set to the value it already had, but the new code looks like it won't do that. If we don't have an existing test that catches this, please write one or let me know and I can whip one up pretty quickly. This should be testable as a web platform test, since it's a spec issue. Though we should double-check what the spec says here, exactly, and maybe what other browsers do... ::: dom/html/nsGenericHTMLFrameElement.cpp:418 (Diff revision 2) > + LoadSrc(); > + } > + } > + } > + > + return; No need. ::: dom/mathml/nsMathMLElement.cpp:1071 (Diff revision 2) > nsCOMPtr<nsIURI> hrefURI; > return IsLink(getter_AddRefs(hrefURI)) ? hrefURI.forget() : nullptr; > } > > nsresult > -nsMathMLElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName, > +nsMathMLElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName, Might as well keep the aNamespaceID naming for the first arg, to reduce churn. ::: dom/mathml/nsMathMLElement.cpp:1087 (Diff revision 2) > - if (aNameSpaceID == kNameSpaceID_XLink) { > + if (aValue && aNamespaceID == kNameSpaceID_XLink) { > WarnDeprecated(u"xlink:href", u"href", OwnerDoc()); > } > - Link::ResetLinkState(!!aNotify, true); > - } > - > + // Note: When unsetting href, there may still be another href since there > + // are 2 possible namespaces. > + Link::ResetLinkState(!!aNotify, aValue ? true : Link::ElementHasHref()); Just pass aNotify for the first arg. For the second arg, maybe `aValue || Link::ElementHasHref()` would be clearer? ::: dom/xbl/XBLChildrenElement.cpp:37 (Diff revision 2) > + const nsAttrValueOrString* aValue, > - bool aNotify) > + bool aNotify) > { > - if (aAttribute == nsGkAtoms::includes && > - aNameSpaceID == kNameSpaceID_None) { > + if (aNamespaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::includes) { > - mIncludes.Clear(); > + mIncludes.Clear(); This is mis-indented. Should only indent by 2.
Cleaned up the first 3 patches and filed Bug 1370705, as requested. Looking at the review of the fourth patch now.
Flags: needinfo?(ksteuber)
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review150376 > Weird indent here, please fix. Can you recommend a fix? No matter how I indent this it looks wrong. Of the options I tried, I actually like the current option the best. > I don't think failures from UpdateContext here should be propagated out to the SetAttr() caller, and hence shouldn't be propagated out of this function. In particular, throwing an exception from setAttribute to script if we just run out of webgl contexts to allocate is pretty weird and not what the specs say to do at all, afaict. > > So we should not propagate this rv out of this function, and hence it can return void. > > In addition to that, none of the nsICanvasRenderingContextInternal::SetIsOpaque implementations return error. Please file a followup to change that function to return void? Filed Bug 1370727. > No, this doesn't do the same thing as the old code. I'd expect tests to fail as a result, if we had tests for this stuff... Unfortunately, bug 48422 predates us having a CI infrastructure or testing or any of that stuff. Do you mind adding some tests that would have caught that? Or I can do it if you prefer; let me know. > > Anyway, the point of this mCurrentRowColHint business is that it's used in HTMLFrameSetElement::GetAttributeChangeHint which is called from the restyle manager AttributeChanged implementations. Which means that it needs to be in the value the "before" hook set it to when we do the call to nsNodeUtils::AttributeChanged in Element::SetAttrAndNotify. But that happens after the AfterSetAttr call. > > Maybe what we should do is set mCurrentRowColHint to NS_STYLE_HINT_REFLOW at the beginning of BeforeSetAttr unconditionally (i.e. no matter what attr is changing). And document that the idea is that the only consumer is GetAttributeChangeHint, which is only called from AttributeChanged notifications, and this way we'll ensure that GetAttributeChangeHint sees NS_STYLE_HINT_REFLOW except in the cases in which our rows/cols are changing and it needs to see the ReconstructFrame hint. > > It's icky, but I don't think it's more icky than what we have here right now... I understand that the lack of other consumers means that it is (relatively) safe to leave the reframe style hint rather than reseting it to reflow. However, I do not understand what effect this has (or really anything about style hints whatsoever). If you help me understand what the test needs to test for, I will file a bug. > This doesn't look right to me. In particular, AfterMaybeChangeAttr just does the DoLoad() bits from SetAttr for the "src" attribute changing, right? Those bits used to happen _after_ the AfterSetAttr code for "src", but after this patch would happen _before_, right? And in particular before mSrcMediaSource is updated. I'm fairly surprised this does not cause any test failures... Might be worth asking one of the media folks to add a test it would have failed. > > Also, we didn't use to DoLoad() from UnsetAttr, so this call really does need to be conditional on aValue, afaict (and the header comments which don't match this code were correct). Not sure whether this is testabl, but if it is we should add a test. Good catch. I am also a bit suprised that no test caught this. If I am reading the code correctly, it appears that setting media to a different source would result in it reloading from the old source rather than the new one, correct? If that sounds right, I'll file a bug to add a test for this. > This is subtly changing the behavior of the "name" attr. We used to propagate it to the docshell even if it were being set to the value it already had, but the new code looks like it won't do that. If we don't have an existing test that catches this, please write one or let me know and I can whip one up pretty quickly. This should be testable as a web platform test, since it's a spec issue. > > Though we should double-check what the spec says here, exactly, and maybe what other browsers do... Filed Bug 1371027
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review150376 > Can you recommend a fix? No matter how I indent this it looks wrong. Of the options I tried, I actually like the current option the best. I'd line up the aNames with each other. So: (aName == nsGkAtoms::width || aName == nsGkAtoms::height || aName == nsGkAtoms::moz_opaque)) { > I understand that the lack of other consumers means that it is (relatively) safe to leave the reframe style hint rather than reseting it to reflow. However, I do not understand what effect this has (or really anything about style hints whatsoever). If you help me understand what the test needs to test for, I will file a bug. I think https://bug48422.bmoattachments.org/attachment.cgi?id=90746 and https://bug48422.bmoattachments.org/attachment.cgi?id=90747 (testcases on bug 48422) are tests that should be failing with the patch I reviewed. Those are manual tests, but an automated reftest could just run that JS to add a row or col after flushing out layout onload. For now, just checking that the attached patch fails those tests but passes with the modified version is a good start. The idea with changehints is to indicate to the layout engine what changes need to happen as a result of various things (attribute changes, changes to selector matching, etc). For the case of changes to the rows and cols attribute of frameset that change the number of frames that we being created, we need to recreate the nsIFrame for the frameset; for changes to those attributes that just adjust sizes we can get away with a relayout. > Good catch. I am also a bit suprised that no test caught this. If I am reading the code correctly, it appears that setting media to a different source would result in it reloading from the old source rather than the new one, correct? If that sounds right, I'll file a bug to add a test for this. I think we have to have an src attr being set, but also both the new and old URLs need to be URL.createObjectURL return values from MediaSource objects being passed in. :karlt or someone else on the media team might be helpful in terms of coming up with a testcase here, though the testcases attached to 1116382 might be a start on at least figuring out how the testcase would be put together.
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review150376 > I think https://bug48422.bmoattachments.org/attachment.cgi?id=90746 and https://bug48422.bmoattachments.org/attachment.cgi?id=90747 (testcases on bug 48422) are tests that should be failing with the patch I reviewed. Those are manual tests, but an automated reftest could just run that JS to add a row or col after flushing out layout onload. For now, just checking that the attached patch fails those tests but passes with the modified version is a good start. > > The idea with changehints is to indicate to the layout engine what changes need to happen as a result of various things (attribute changes, changes to selector matching, etc). For the case of changes to the rows and cols attribute of frameset that change the number of frames that we being created, we need to recreate the nsIFrame for the frameset; for changes to those attributes that just adjust sizes we can get away with a relayout. Filed Bug 1371073 > I think we have to have an src attr being set, but also both the new and old URLs need to be URL.createObjectURL return values from MediaSource objects being passed in. > > :karlt or someone else on the media team might be helpful in terms of coming up with a testcase here, though the testcases attached to 1116382 might be a start on at least figuring out how the testcase would be put together. Would you mind filing the bug for this? I do not feel like I have a solid grasp on the problem.
I think I have addressed all your concerns. How does it look now?
Flags: needinfo?(bzbarsky)
Priority: -- → P3
Blocks: 1371073
Flags: needinfo?(bzbarsky)
Depends on: 1371158
Blocks: 1371158
No longer depends on: 1371158
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review150376 > Would you mind filing the bug for this? I do not feel like I have a solid grasp on the problem. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1371158
Comment on attachment 8873640 [details] Bug 1365092 - Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr https://reviewboard.mozilla.org/r/145032/#review151110 r=me
Attachment #8873640 - Flags: review+
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cde92b45e56c Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions r=bz https://hg.mozilla.org/integration/autoland/rev/a52c6e64be6e Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions r=bz https://hg.mozilla.org/integration/autoland/rev/c0a03c5be172 Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions r=bz https://hg.mozilla.org/integration/autoland/rev/f5da859b433f Move side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr r=bz
Depends on: 1402025
Depends on: 1384606
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: