Dynamic change of maction actiontype attribute has no effect. Possibly it is because mActionType is initialized in nsMathMLmactionFrame::Init and never changed afterwards. Possible solution is to override nsMathMLContainerFrame::AttributeChanged in nsMathMLmactionFrame and update mActionType there. The testcase demonstrates the issue.

Sorry, first testcase was uploaded improperly.

In here, nsMathMLContainerFrame::AttributeChanged was overridden in nsMathMLmactionFrame to update mActionType, also I made some optimization by checking whether it's needed to do a reflow or not. nsMathMLmactionFrame::Init was cleaned up a bit as well. There is still one problem. Although it seemed that AttributeChanged should work only with kNameSpaceID_MathML, it doesn't seem to work (see #if 0 section). Also there is a question about namespace used in all mContent->GetAttr or mContent->SetAttr calls in nsMathMLmactionFrame. It's kNameSpaceID_None there, but shouldn't it be kNameSpaceID_MathML?

> There is still one problem. Although it seemed that AttributeChanged should > work only with kNameSpaceID_MathML, it doesn't seem to work (see #if 0 > section). Also there is a question about namespace used in all > mContent->GetAttr or mContent->SetAttr calls in nsMathMLmactionFrame. It's > kNameSpaceID_None there, but shouldn't it be kNameSpaceID_MathML? You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so that can not work. I think for the attributes, kNameSpaceID_None should. For example in the SVG code: http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGImageFrame.cpp#216

> You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so > that can not work. I tried using only kNameSpaceID_MathML everywhere in maction, but it didn't help.

(In reply to Andrii Zui from comment #4) > > You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so > > that can not work. > > I tried using only kNameSpaceID_MathML everywhere in maction, but it didn't > help. Yes, since apparently the correct syntax for attributes is without namespace. Otherwise, I guess you also have to specify the MathML namespace in a SetAttributeNS of your tests.

Comment on attachment 615865 [details] [diff] [review] override nsMathMLContainerFrame::AttributeChanged >+PRInt32 >+nsMathMLmactionFrame::ParseActionType(const nsAutoString& value) This method doesn't need access to the nsMathMLmactionFrame object or class so make this a plain old static linkage function. If you pass in the nsIContent* instead of the value then the GetAttr code can be in this method instead of in both of the callers. >+ if ((mActionType == NS_MATHML_ACTION_TYPE_STATUSLINE || >+ mActionType == NS_MATHML_ACTION_TYPE_TOOLTIP) && >+ (oldActionType == NS_MATHML_ACTION_TYPE_STATUSLINE || >+ oldActionType == NS_MATHML_ACTION_TYPE_TOOLTIP)) { >+ needsReflow = false; Do we only need to reflow when either the old or new action type is toggle? The optimizations would look more worthwhile if needsReflow defaulted to false and was set to true only in a few cases. >+ } else if (aAttribute == nsGkAtoms::selection_) { >+ // When the selection attribute is changed we have to initiate a reflow >+ // only when actiontype is toggle. >+ if (NS_MATHML_ACTION_TYPE_TOGGLE != mActionType) >+ needsReflow = false; >+ } else { Please use braces around the NS_MATHML_ACTION_TYPE_TOGGLE != mActionType block (to make it clear that the nested if/else blocks are as intended).

I updated the patch with Karl's comments taken into account. I believe that in nsMathMLmactionFrame::AttributeChanged it's enough to call FrameNeedsReflow with eTreeChange instead of eStyleChange. But I'm not much familiar with it, so I can be mistaken. Also, I merged the GetAttr call into GetActionType function (renamed from ParseActionType in the previous version of the patch). It will take actiontype attribute from kNameSpaceID_None, I hope it's ok since the call of GetAttr in nsMathMLmactionFrame::Init was referring to kNameSpaceID_None as well.

Comment on attachment 617255 [details] [diff] [review] patch v2: optimization of AttributeChanged function >+PRInt32 >+GetActionType(nsIContent* aContent) Include the static keyword here for internal linkage. Otherwise, this looks good, thanks.

Comment on attachment 617455 [details] [diff] [review] patch v3: optimization of AttributeChanged function Can you modify the checkin comment a bit, please? The key change is that this now handles dynamic changes to the actiontype attribute, rather than optimization of an existing functionality. The bug number should also be included. Perhaps "b=617455 Implement AttributeChanged on nsMathMLmactionFrame r=karlt".

Fixed. Haven't thought about the description much, sorry about that.

Andrii, can you please also attach to this bug the dynamic reftests you wrote last week (and ask review)? That will allow to test your patch (if I remember well) when you send your patch to the try server.

Autoland Patchset: Patches: 618011, 622097 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=87e2cdb9d86a Try run started, revision 87e2cdb9d86a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=87e2cdb9d86a

(In reply to Mozilla RelEng Bot from comment #14) > Autoland Patchset: > Patches: 618011, 622097 > Branch: mozilla-central => try > Destination: http://hg.mozilla.org/try/pushloghtml?changeset=87e2cdb9d86a > Try run started, revision 87e2cdb9d86a. To cancel or monitor the job, see: > https://tbpl.mozilla.org/?tree=Try&rev=87e2cdb9d86a The results look good. I don't know why the bot takes so much time to post a message indicating the end...

https://hg.mozilla.org/integration/mozilla-inbound/rev/05a02c6ec1b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a321bf58ce

https://hg.mozilla.org/mozilla-central/rev/05a02c6ec1b6 https://hg.mozilla.org/mozilla-central/rev/c3a321bf58ce