Last Comment Bug 745535 - dynamic change of the maction actiontype attribute doesn't work
: dynamic change of the maction actiontype attribute doesn't work
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrii Zui
:
Mentors:
Depends on:
Blocks: maction 744783
  Show dependency treegraph
 
Reported: 2012-04-15 06:21 PDT by Andrii Zui
Modified: 2012-05-18 18:23 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (499 bytes, text/plain)
2012-04-15 06:21 PDT, Andrii Zui
no flags Details
testcase (499 bytes, text/html)
2012-04-15 06:25 PDT, Andrii Zui
no flags Details
override nsMathMLContainerFrame::AttributeChanged (6.40 KB, patch)
2012-04-17 14:00 PDT, Andrii Zui
karlt: feedback+
Details | Diff | Review
patch v2: optimization of AttributeChanged function (5.41 KB, patch)
2012-04-21 13:33 PDT, Andrii Zui
no flags Details | Diff | Review
patch v3: optimization of AttributeChanged function (5.41 KB, patch)
2012-04-23 06:23 PDT, Andrii Zui
karlt: review+
Details | Diff | Review
patch v4: Implement AttributeChanged on nsMathMLmactionFrame (5.41 KB, patch)
2012-04-24 13:18 PDT, Andrii Zui
karlt: review+
Details | Diff | Review
dynamic reftests (7.17 KB, patch)
2012-05-08 12:48 PDT, Andrii Zui
karlt: review+
Details | Diff | Review

Description Andrii Zui 2012-04-15 06:21:11 PDT
Created attachment 615141 [details]
testcase

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.
Comment 1 Andrii Zui 2012-04-15 06:25:36 PDT
Created attachment 615142 [details]
testcase

Sorry, first testcase was uploaded improperly.
Comment 2 Andrii Zui 2012-04-17 14:00:30 PDT
Created attachment 615865 [details] [diff] [review]
override nsMathMLContainerFrame::AttributeChanged

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?
Comment 3 Frédéric Wang (:fredw) 2012-04-18 00:59:21 PDT
> 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
Comment 4 Andrii Zui 2012-04-18 05:12:01 PDT
> 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.
Comment 5 Frédéric Wang (:fredw) 2012-04-18 06:14:29 PDT
(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 6 Karl Tomlinson (ni?:karlt) 2012-04-18 22:44:53 PDT
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).
Comment 7 Andrii Zui 2012-04-21 13:33:06 PDT
Created attachment 617255 [details] [diff] [review]
patch v2: optimization of AttributeChanged function

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 8 Karl Tomlinson (ni?:karlt) 2012-04-22 19:11:02 PDT
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 9 Andrii Zui 2012-04-23 06:23:38 PDT
Created attachment 617455 [details] [diff] [review]
patch v3: optimization of AttributeChanged function
Comment 10 Karl Tomlinson (ni?:karlt) 2012-04-23 15:29:24 PDT
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".
Comment 11 Andrii Zui 2012-04-24 13:18:46 PDT
Created attachment 618011 [details] [diff] [review]
patch v4: Implement AttributeChanged on nsMathMLmactionFrame

Fixed. Haven't thought about the description much, sorry about that.
Comment 12 Frédéric Wang (:fredw) 2012-04-26 02:38:04 PDT
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.
Comment 13 Andrii Zui 2012-05-08 12:48:02 PDT
Created attachment 622097 [details] [diff] [review]
dynamic reftests
Comment 14 Mozilla RelEng Bot 2012-05-11 16:07:19 PDT
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
Comment 15 Frédéric Wang (:fredw) 2012-05-13 10:27:01 PDT
(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...

Note You need to log in before you can comment on or make changes to this bug.