As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
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 | Splinter Review
patch v2: optimization of AttributeChanged function (5.41 KB, patch)
2012-04-21 13:33 PDT, Andrii Zui
no flags Details | Diff | Splinter Review
patch v3: optimization of AttributeChanged function (5.41 KB, patch)
2012-04-23 06:23 PDT, Andrii Zui
karlt: review+
Details | Diff | Splinter Review
patch v4: Implement AttributeChanged on nsMathMLmactionFrame (5.41 KB, patch)
2012-04-24 13:18 PDT, Andrii Zui
karlt: review+
Details | Diff | Splinter Review
dynamic reftests (7.17 KB, patch)
2012-05-08 12:48 PDT, Andrii Zui
karlt: review+
Details | Diff | Splinter Review

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

Sorry, first testcase was uploaded improperly.
Comment 2 User image 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 User image 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 User image 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 User image 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 User image Karl Tomlinson (back Feb 1 :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 User image 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 User image Karl Tomlinson (back Feb 1 :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 User image Andrii Zui 2012-04-23 06:23:38 PDT
Created attachment 617455 [details] [diff] [review]
patch v3: optimization of AttributeChanged function
Comment 10 User image Karl Tomlinson (back Feb 1 :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 User image 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 User image 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 User image Andrii Zui 2012-05-08 12:48:02 PDT
Created attachment 622097 [details] [diff] [review]
dynamic reftests
Comment 14 User image 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 User image 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.