Last Comment Bug 673759 - (namedspaceOverriding) [MathML3] Remove namedspace value overriding
(namedspaceOverriding)
: [MathML3] Remove namedspace value overriding
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Frédéric Wang (:fredw)
:
:
Mentors:
Depends on: 677036 696647
Blocks: mathml-3
  Show dependency treegraph
 
Reported: 2011-07-24 01:46 PDT by Frédéric Wang (:fredw)
Modified: 2012-06-06 12:12 PDT (History)
4 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Unification of nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (31.63 KB, patch)
2011-07-28 04:44 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Reftest for positive namedspaces (3.34 KB, patch)
2011-07-28 04:45 PDT, Jonathan Hage
karlt: review+
emorley: checkin+
Details | Diff | Splinter Review
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (30.16 KB, patch)
2011-07-28 06:00 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (28.64 KB, patch)
2011-08-02 00:28 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (28.38 KB, patch)
2011-08-02 04:41 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding v2 (31.59 KB, patch)
2011-08-08 01:42 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
no flags Details | Diff | Splinter Review Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (30.16 KB, patch) 2011-07-28 06 (31.51 KB, patch)
2011-08-08 01:54 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding v2 (31.51 KB, patch)
2011-08-08 03:50 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch V3 - part 1 (28.82 KB, patch)
2012-01-05 08:53 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 - part 2 (5.49 KB, patch)
2012-01-05 08:57 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2011-07-24 01:46:05 PDT
The REC says:

"MathML2 allowed the binding of namedspaces to new values. It appears that this capability was never implemented, and is now deprecated; namedspaces are now considered constants. For backwards compatibility, the following attributes are accepted on the mstyle element, but are expected to have no effect."

We implement namedspace value overriding in nsMathMLFrame::ParseNamedSpaceValue. Removing this will allow to get rid of many almost never used nsGkAtoms for namespaces. This will also simplify the parsing code: we could move the parsing for namespaces into nsMathMLElement::ParseNumericValue (and add a flag PARSE_ALLOW_NAMEDSPACE).
Comment 1 Jonathan Hage 2011-07-28 04:44:13 PDT
Created attachment 549072 [details] [diff] [review]
Unification of nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding
Comment 2 Jonathan Hage 2011-07-28 04:45:10 PDT
Created attachment 549073 [details] [diff] [review]
Reftest for positive namedspaces
Comment 3 Jonathan Hage 2011-07-28 06:00:13 PDT
Created attachment 549088 [details] [diff] [review]
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding
Comment 4 Jonathan Hage 2011-08-02 00:28:06 PDT
Created attachment 550018 [details] [diff] [review]
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding
Comment 5 Jonathan Hage 2011-08-02 04:41:22 PDT
Created attachment 550043 [details] [diff] [review]
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding
Comment 6 Frédéric Wang (:fredw) 2011-08-03 13:47:27 PDT
(In reply to comment #5)
> Created attachment 550043 [details] [diff] [review] [diff] [details] [review]
> Moving nsMathMLFrame::ParseNamedSpaceValue into
> nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding

Compilation fails with this patch:

/src-obj/mozilla/src/content/base/src/nsTreeSanitizer.cpp:952: error: ‘negativemediummathspace_’ is not a member of ‘nsGkAtoms’
...

The nsGkAtoms for spaces should be removed everywhere.
Comment 7 Jonathan Hage 2011-08-08 01:42:15 PDT
Created attachment 551404 [details] [diff] [review]
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding v2
Comment 8 Jonathan Hage 2011-08-08 01:54:03 PDT
Created attachment 551407 [details] [diff] [review]
no flags 	Details | Diff | Splinter Review Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding (30.16 KB, patch) 2011-07-28 06
Comment 9 Jonathan Hage 2011-08-08 03:50:08 PDT
Created attachment 551414 [details] [diff] [review]
Moving nsMathMLFrame::ParseNamedSpaceValue into nsMathMLElement::ParseNumericValue + Removing of namedspace value overriding v2
Comment 10 Frédéric Wang (:fredw) 2011-09-24 12:03:40 PDT
Comment on attachment 549073 [details] [diff] [review]
Reftest for positive namedspaces

We can probably already take this reftest.
Comment 11 Frédéric Wang (:fredw) 2011-09-26 09:22:07 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=74d9def9c9de
Comment 12 Ed Morley [:emorley] 2011-09-26 19:16:23 PDT
I'm presuming this needs to be left open for the other patch? Should a review be set on that patch?

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ce0b70c2e1
Comment 13 Ed Morley [:emorley] 2011-09-27 03:28:33 PDT
Comment on attachment 549073 [details] [diff] [review]
Reftest for positive namedspaces

https://hg.mozilla.org/mozilla-central/rev/f5ce0b70c2e1
Comment 14 Ed Morley [:emorley] 2011-09-27 03:29:12 PDT
Remaining patch isn't going to make it for mozilla9, so removing target milestone for now.
Comment 15 Frédéric Wang (:fredw) 2012-01-05 08:53:16 PST
Created attachment 586105 [details] [diff] [review]
Patch V3 - part 1
Comment 16 Frédéric Wang (:fredw) 2012-01-05 08:57:56 PST
Created attachment 586107 [details] [diff] [review]
Patch V3 - part 2

Part 2, removing the corresponding MathML Atoms. To apply over patch from bug 696647.

I'm not sure this patch is good idea, though. Bug 324472 comment 5 says that atoms are needed for clipboard operations.
Comment 17 Frédéric Wang (:fredw) 2012-01-05 08:58:21 PST
https://tbpl.mozilla.org/?tree=Try&rev=bf7895f8e2fb
Comment 18 Frédéric Wang (:fredw) 2012-01-05 13:59:48 PST
Comment on attachment 586105 [details] [diff] [review]
Patch V3 - part 1

Note: I have not changed the flags passed to ParseNumericValue, as I expect this job to be done in bug 411227.
Comment 19 Frédéric Wang (:fredw) 2012-05-17 00:27:34 PDT
Comment on attachment 586107 [details] [diff] [review]
Patch V3 - part 2

So now bug 677036 is fixed, it should not be possible to override namedspace. Karl, do you think we should take attachment 586107 [details] [diff] [review]? Bug 324472 comment 5 indicates that atoms are needed for clipboard operations. However, I don't think people have ever used namedspace overriding, so that should not be a problem?
Comment 20 Karl Tomlinson (:karlt) 2012-05-17 01:14:13 PDT
Comment on attachment 586107 [details] [diff] [review]
Patch V3 - part 2

Yes, makes sense to remove these now, thanks.
Comment 21 Frédéric Wang (:fredw) 2012-05-18 02:09:39 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5adfc5ff68c7

I'm not sure how the build results can be red (with empty summaries) but the tests are executed and seem successful.
Comment 22 Frédéric Wang (:fredw) 2012-05-21 12:54:06 PDT
(In reply to Frédéric Wang (:fredw) from comment #21)
> https://tbpl.mozilla.org/?tree=Try&rev=5adfc5ff68c7
> 
> I'm not sure how the build results can be red (with empty summaries) but the
> tests are executed and seem successful.

Karl, do you have any idea about this problem? I can build opt/debug builds without problems with this patch applied. I tried the patch twice on the Try server and always got this build results in red, with empty summaries.
Comment 23 Karl Tomlinson (:karlt) 2012-05-22 01:13:14 PDT
I don't know what happened there but some others had similar problems
e.g. https://tbpl.mozilla.org/?tree=Try&rev=be00392bbc1f

I'd try pulling and rebasing against the latest known good changeset.
Comment 24 Frédéric Wang (:fredw) 2012-05-22 08:27:56 PDT
OK, that looks better:
https://tbpl.mozilla.org/?tree=Try&rev=3dc56ac6f8c3
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-05-22 17:25:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a048bf8506f7
Comment 26 Ed Morley [:emorley] 2012-05-23 04:53:46 PDT
https://hg.mozilla.org/mozilla-central/rev/a048bf8506f7

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