Redesign how we send change notifications from SVG types

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jwatt, Assigned: birtles)

Tracking

(Blocks: 3 bugs, {perf})

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(28 attachments, 116 obsolete attachments)

628 bytes, image/svg+xml
Details
1.97 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
4.19 KB, text/plain
Details
3.63 KB, text/plain
Details
10.60 KB, patch
Details | Diff | Splinter Review
13.26 KB, patch
Details | Diff | Splinter Review
33.27 KB, patch
Details | Diff | Splinter Review
34.09 KB, patch
Details | Diff | Splinter Review
9.13 KB, patch
Details | Diff | Splinter Review
16.90 KB, patch
Details | Diff | Splinter Review
9.62 KB, patch
Details | Diff | Splinter Review
17.44 KB, patch
Details | Diff | Splinter Review
22.60 KB, patch
Details | Diff | Splinter Review
10.11 KB, patch
Details | Diff | Splinter Review
6.63 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
17.78 KB, patch
Details | Diff | Splinter Review
14.78 KB, patch
Details | Diff | Splinter Review
20.78 KB, patch
Details | Diff | Splinter Review
21.46 KB, patch
Details | Diff | Splinter Review
22.33 KB, patch
Details | Diff | Splinter Review
28.25 KB, patch
Details | Diff | Splinter Review
303.11 KB, patch
Details | Diff | Splinter Review
21.76 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
21.77 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
47.48 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
4.57 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
29.78 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
27.51 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Currently we have lots of DidChangeXxx methods for sending out notifications when the SVG DOM is used to change SVGPathSegList objects etc.:

http://hg.mozilla.org/mozilla-central/annotate/8b600831f115/content/svg/content/src/nsSVGElement.h#l176

The problem is that these methods call SetParsedAttr after the change has occurred, but under SetParsedAttr we call AttributeWillChange. For restyles to work correctly GetAttr needs to return the old value under AttributeWillChange, so our nsAttrValue can't point to and return the serialization of the SVG object that just changed. The way we handle this currently is to store the serialization of the SVG object in the nsAttrValue object so that under SetParsedAttr, GetAttr will be able to return the serialization of the old value instead of the serialization of the current object's value. The problem with this approach is that we need to serialize the SVG object (in the case of lists this can be a lot of serialization) every time the SVG object is changed, which in the case of a script going through building up a list or changing all its items can be a lot of times. It also means that we're using memory to store serializations that would otherwise be unnecessary.

If we can redesign our notification system to send separate WillModify/DidModify notifications we should hopefully be able to get rid of all this serialization and extra memory use.
It's not just restyles for which we need to be able to get the old value under SetParsedAttr, it's also for mutation events, if they're sent.
(Reporter)

Updated

7 years ago
Keywords: perf
I thought we'd also need to modify nsGenericElement::BeforeSetAttr and AfterSetAttr to take an nsAttrValue instead of a string, but it looks like we don't once bug 602759 is fixed, because at that point no SVG elements actually need BeforeSetAttr/AfterSetAttr to be called. Instead we simply need an attribute-setting path that doesn't call BeforeSetAttr/AfterSetAttr.
Version: unspecified → Trunk
(Assignee)

Comment 3

7 years ago
Taking this as it seems to be the root cause of bug 646510 which is assigned to me.
Assignee: nobody → birtles
Blocks: 646510
Status: NEW → ASSIGNED
Depends on: 602759
Note: when fixing this take care that the mIsBaseSet/mIsAttrSet/mAttrIsSet members of SVG types continue to be set correctly.
(Assignee)

Comment 5

6 years ago
I'm just starting to dig into this. One clarification to check I'm on the right track--I guess we want to be able to wrap our SVG types in an nsAttrValue? I think we used to be able to do that with nsISVGValue but that interface is on the way out (in fact, after bug 602759 lands, I think only nsSVGStringProxyValue will use that interface). So perhaps we need another lightweight equivalent of that interface?
Yes. We may not need a common interface though, we can add as many new types as we need to ValueType with corresponding fields to MiscContainer.

I think the cleanest way to do this (without creating new attribute-setting code paths that bypass BeforeSetAttr/AfterSetAttr sometimes) is to change nsGenericElement::SetParsedAttr to not serialize the new value to a string. I think this would require, at least:
-- Allowing MaybeCheckSameAttrVal to work on parsed attributes instead of strings
-- Changing BeforeSetAttr/AfterSetAttr to take nsAttrValues instead of strings

I seem to remember more discussion about this in some other bug.
(Assignee)

Comment 7

6 years ago
Created attachment 563626 [details] [diff] [review]
WIP part 1 - Refactor MaybeCheckSameAttrVal

Just posting a bunch of WIP patches because I'm going to be away for a while.

Basically so far I've done enums and lengths. Next is length lists. If those three turn out ok then I hope it will be fairly straightforward to update the other types by following a similar approach.
(Assignee)

Comment 8

6 years ago
Created attachment 563627 [details] [diff] [review]
WIP part 2 - Refactor BeforeSetAttr
(Assignee)

Comment 9

6 years ago
Created attachment 563628 [details] [diff] [review]
WIP part 3 - Refactor AfterSetAttr
(Assignee)

Comment 10

6 years ago
Created attachment 563629 [details] [diff] [review]
WIP part 4 - Name some bools to make things easier to read
(Assignee)

Comment 11

6 years ago
Created attachment 563630 [details] [diff] [review]
WIP part 5 - Add test framework for modification events on simple SVG types
(Assignee)

Comment 12

6 years ago
Created attachment 563631 [details] [diff] [review]
WIP part 6 - Refactor SVGEnums

There's one test failure with this patch, but it's an existing bug I've yet to dig into (about not sending an "addition" notification when we touch the DOM interface of an enum that previously wasn't set)
(Assignee)

Comment 13

6 years ago
Created attachment 563632 [details] [diff] [review]
WIP part 7 - Refactor nsSVGLength2
(Assignee)

Comment 14

6 years ago
Created attachment 563633 [details] [diff] [review]
WIP part X - Tests for other SVG types yet to be refactored
Blocks: 608495
Brian,

I'm removing all the overrides of DidChangeXXX so that the versions in nsSVGElement can become non-virtual. That will save around 128 bytes for each element we create in the SVG namespace. The changes are not in direct conflict with what you're doing but they will bitrot these patches each time one of mine lands as you're modifying the overrides that I'm gradually removing. Once I've finished there won't be any overrides to modify. I have patches either landed or in review for everything except nsSVGFilters so far.
On reflection and with jwatt's help, that's 128 bytes per class not per instance.
(Assignee)

Comment 17

6 years ago
(In reply to Robert Longson from comment #15)
> Brian,
> 
> I'm removing all the overrides of DidChangeXXX so that the versions in
> nsSVGElement can become non-virtual. That will save around 128 bytes for
> each element we create in the SVG namespace. The changes are not in direct
> conflict with what you're doing but they will bitrot these patches each time
> one of mine lands as you're modifying the overrides that I'm gradually
> removing. Once I've finished there won't be any overrides to modify. I have
> patches either landed or in review for everything except nsSVGFilters so far.

Hi Robert,

Thanks for the heads up. I'm currently on break, back in action next Monday. I'm already anticipating spending a bit of time unbitrotting this stuff so if you manage to get it landed by next week then it's absolutely no problem. Otherwise, I'll just keep in mind those changes and get ready to jump on board when they land.

Thanks again.
(Assignee)

Comment 18

6 years ago
I'm unbitrotting this but I've got a new test failure (in the newly added code and tests) due to bug 550047.

Basically we used to run nsGenericElement::UnsetAttr BEFORE clearing the SVG data type (an nsSVGLength2 in the case I am debugging). So when we dispatched MutationEvent (AttrModified) for the attribute removal, the copied nsAttrValue we use to fill in the prevValue field of the event still pointed to the nsSVGLength2 with its value set. After nsGenericElement::UnsetAttr returned we'd go ahead and clear it.

Since bug 550047 we now clear the internal SVG data types BEFORE calling nsGenericElement::UnsetAttr so when we go to dispatch the event the nsAttrValue now points to a cleared nsSVGLength2 and the prevValue field ends up being wrong.

I'm thinking of doing:

A) In nsSVGElement::UnsetAttrInternal, detect if we're going to dispatch an AttrModified event and, if so, serialise the attr value before clearing the SVG type. That way we should still get the right value in the prevValue for the (rare) case that the attribute has mutation listeners.

I'm writing this up because I'm a little unsure about it. The serialisation seems a little hacky (although if we're dispatching the event, we're going to do some *some* serialisation, it's just now we end up using two string buffers).

Also, it seems odd to call all these AttributeWillChange / BeforeSetAttr methods inside nsGenericElement::UnsetAttr when in fact the attribute has already changed (in the current case the SVG data type is already cleared and once we make nsAttrValues point to the SVG data types, the attribute too will have already been updated in effect. Even with the serialisation proposed in (A) the attribute will have at least changed format.)

Somehow, it seems more consistent to have listeners to AttributeChanged and the like actually just look up the attribute in question, and if there's no nsAttrValue then act as if it is not set, rather than bypassing the attributes and going straight to the SVG internal types. But I wonder if AttributeChanged is triggering a lot of synchronous updates that all look up the SVG types directly.

For now I'm going to give (A) a shot.
(Assignee)

Comment 19

6 years ago
Created attachment 570943 [details] [diff] [review]
WIP patch 2

WIP patch 2. This time all folded up into one for easier cross-reference.

* Unbitrotted
* All tests now pass
* SVGEnum, SVGLength, SVGLengthList now done

Still about another dozen or so types to go but I think the above three are representative of the remaining types so hopefully it's just a matter of copying what they do and hopefully won't take too long.
Attachment #563626 - Attachment is obsolete: true
Attachment #563627 - Attachment is obsolete: true
Attachment #563628 - Attachment is obsolete: true
Attachment #563629 - Attachment is obsolete: true
Attachment #563630 - Attachment is obsolete: true
Attachment #563631 - Attachment is obsolete: true
Attachment #563632 - Attachment is obsolete: true
Attachment #563633 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #570943 - Attachment description: WIP patch 1 → WIP patch 2
do you need the aDoSetAttr argument in Will/DidChange. Can't you just not call the method at all if the argument is false?
(Assignee)

Comment 21

6 years ago
(In reply to Robert Longson from comment #20)
> do you need the aDoSetAttr argument in Will/DidChange. Can't you just not
> call the method at all if the argument is false?

I definitely want to get rid of that and do like you say. Were you going to make all those DidChangeXXX no longer virtual? If that's the case that would give me confidence to drop the unnecessary calls. Otherwise I need to go through and make sure we're not overriding DidChangeXXX anywhere before dropping the call.
bz, Jonas, it would be great if one of you (or both of you) can look at Brian's patch to make sure he's going in the right direction. Especially the nsGenericElement changes.
The nsGenericElement changes seem generally sane to me; the one concern is whether we end up making more string copies on setAttribute calls from JS, but I'm not sure we do in fact...
Blocks: 698996
(Assignee)

Comment 24

6 years ago
(In reply to Robert Longson from comment #20)
> do you need the aDoSetAttr argument in Will/DidChange. Can't you just not
> call the method at all if the argument is false?

For what it's worth, I've started dropping these calls (and removing the aDoSetAttr) and making them non-virtual. Will post another patch on Friday--Thurs is public holiday here.
(Assignee)

Comment 25

6 years ago
Created attachment 571904 [details] [diff] [review]
WIP patch 3

Updated WIP patch.

Still to do:
* nsSVGViewBox
* SVGAnimatedNumberList
* SVGAnimatedPointList
* SVGAnimatedPathSegList
* SVGAnimatedTransformList
* See if I can make some sort of adaptor class for the SVG types so as not to pollute nsAttrValue with all these SVG types

It's one big patch here but in 17 pieces in my queue. I'll post the pieces once it's about done.
Attachment #570943 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Created attachment 572750 [details] [diff] [review]
WIP patch 4

Updated WIP patch.

All types now done. Still to do:
- Track down an apparent leak on windows 7 debug mochitest-chrome
- Track down a test failure on one of the newly added tests on linux
- Review all patches

I did a rough perf comparison this afternoon between a recent try server build for this patch and the latest nightly with the test URL from bug 646510 and got the following results:
Nightly build:  ~13s until rendering complete + ~3s until browser becomes responsive
TryServer build: <2s until rendering complete, immediately responsive

I've made a few changes since but hopefully that's still more or less accurate.
Attachment #571904 - Attachment is obsolete: true
Does it speed up Santa's Workshop at all?
It looks like this depends on bug 696078 and bug 698195 There are overrides to DidChangeValue in nsSVGFilters that need to go away before you can make them non-virtual. bug 696210 is also in the same area and can only land after bug 696078 and bug 698195), maybe this bug superseeds that though.
(Assignee)

Comment 29

6 years ago
(In reply to Robert Longson from comment #28)
> It looks like this depends on bug 696078 and bug 698195 There are overrides
> to DidChangeValue in nsSVGFilters that need to go away before you can make
> them non-virtual. bug 696210 is also in the same area and can only land
> after bug 696078 and bug 698195), maybe this bug superseeds that though.

I think bug 696078 is only dealing with changes to DidAnimateXXX? So no conflict?

This patch doesn't touch DidChangeString--it leaves it as virtual so it shouldn't be dependent on bug 698195.

Having a look at bug 696210, that bug's patch and this work overlap significantly. About half of the patch for bug 696210 is covered by this patch (the missing code being mostly the changes to DidChangeString and DidAnimateXXX).
We can either:
a) Land bug 696210 as is (after fixing bitrot from bug 698630) and rework this bug to fit on top, or
b) Alter bug 696210 to just deal with the DidAnimateXXX changes, then deal with DidChangeString in bug 698195 and the rest of the DidChangeXXX methods here. That would save you having to fix most of the bitrot caused by bug 698630.

I learn towards (b) but am ok with (a). What do you reckon?
(Assignee)

Comment 30

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Does it speed up Santa's Workshop at all?

Will test tomorrow.
I've already fixed the bitrot in bug 698630 locally.
(Assignee)

Comment 32

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Does it speed up Santa's Workshop at all?

Nope. No difference.

I did some quick profiling of Santa's Workshop on Windows 7 with a nightly build and I get:
d2d1.dll 32~34% of samples
xul.dll  24~27%

Within xul.dll 25~33% of samples occur within mozilla::SVGPathData::ConstructPath. The next in the list is nsIFrame::InvalidateInteral with about 5%.

There's really not any serialisation going on in that test case so I guess this bug isn't going to help. It also doesn't seem to help with the helicopter test case for similar reasons.
Oh well. Thanks.
(Assignee)

Comment 34

6 years ago
(In reply to Robert Longson from comment #31)
> I've already fixed the bitrot in bug 698630 locally.

Ok, in that case bug 696210 may as well land before this one since this one will probably take longer to review.
(Assignee)

Comment 35

6 years ago
Created attachment 573107 [details] [diff] [review]
Part 1 - Refactor MaybeCheckSameAttrVal to work on parsed attributes instead of strings
Attachment #572750 - Attachment is obsolete: true
Attachment #573107 - Flags: review?(bzbarsky)
(Assignee)

Comment 36

6 years ago
Created attachment 573108 [details] [diff] [review]
Part 2 - Make BeforeSetAttr take an nsAttrValue
Attachment #573108 - Flags: review?(bzbarsky)
(Assignee)

Comment 37

6 years ago
Created attachment 573109 [details] [diff] [review]
Part 3 - Make AfterSetAttr take an nsAttrValue
Attachment #573109 - Flags: review?(bzbarsky)
(Assignee)

Comment 38

6 years ago
Created attachment 573110 [details] [diff] [review]
Part 4 - Add some named bool parameters for readability
Attachment #573110 - Flags: review?(bzbarsky)
(Assignee)

Comment 39

6 years ago
Created attachment 573111 [details] [diff] [review]
Part 5 - Add an assignment operator to nsAttrValue

We either need to add an assignment operator to nsAttrValue (what this patch does) or disable it (i.e. make it private). Otherwise it's really easy to shoot yourself in the foot by assigning one nsAttrValue object--currently if you do that when the payload is a string the nsStringBuffer refcount won't get incremented and will therefore may eventually get deallocated while nsAttrValues still point to it.
Attachment #573111 - Flags: review?(bzbarsky)
(Assignee)

Comment 40

6 years ago
Created attachment 573112 [details] [diff] [review]
Part 6 - Add a test framework for mutation events
Attachment #573112 - Flags: review?(jwatt)
(Assignee)

Comment 41

6 years ago
Created attachment 573113 [details] [diff] [review]
Part 7 - Remove unnecessary serialisation from setting SVGEnum
Attachment #573113 - Flags: review?(jwatt)
(Assignee)

Comment 42

6 years ago
Created attachment 573114 [details] [diff] [review]
Part 8 - Remove unnecessary serialisation from setting nsSVGLength2
Attachment #573114 - Flags: review?(jwatt)
(Assignee)

Comment 43

6 years ago
Created attachment 573115 [details] [diff] [review]
Part 9 - Remove unnecessary serialisation from setting SVGAnimatedLengthList
Attachment #573115 - Flags: review?(jwatt)
(Assignee)

Comment 44

6 years ago
Created attachment 573116 [details] [diff] [review]
Part 10 - Remove unnecessary serialisation from setting nsSVGNumber2
Attachment #573116 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573114 - Attachment is patch: true
(Assignee)

Comment 45

6 years ago
Created attachment 573117 [details] [diff] [review]
Part 11 - Remove unnecessary serialisation from setting nsSVGNumberPair
Attachment #573117 - Flags: review?(jwatt)
(Assignee)

Comment 46

6 years ago
Created attachment 573118 [details] [diff] [review]
Part 12 - Remove unnecessary serialisation from setting nsSVGInteger
Attachment #573118 - Flags: review?(jwatt)
(Assignee)

Comment 47

6 years ago
Created attachment 573119 [details] [diff] [review]
Part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair
Attachment #573119 - Flags: review?(jwatt)
(Assignee)

Comment 48

6 years ago
Created attachment 573120 [details] [diff] [review]
Part 14 - Remove unnecessary serialisation from setting nsSVGAngle
Attachment #573120 - Flags: review?(jwatt)
(Assignee)

Comment 49

6 years ago
Created attachment 573121 [details] [diff] [review]
Part 15 - Remove unnecessary serialisation from setting nsSVGBoolean
Attachment #573121 - Flags: review?(jwatt)
(Assignee)

Comment 50

6 years ago
Created attachment 573122 [details] [diff] [review]
Part 16 - Add mutation event tests for strings and classes
Attachment #573122 - Flags: review?(jwatt)
(Assignee)

Comment 51

6 years ago
Created attachment 573123 [details] [diff] [review]
Part 17 - Remove unnecessary serialisation from setting SVGPreserveAspectRatio
Attachment #573123 - Flags: review?(jwatt)
(Assignee)

Comment 52

6 years ago
Created attachment 573124 [details] [diff] [review]
Part 18 - Remove unnecessary serialisation from setting nsSVGViewBox
Attachment #573124 - Flags: review?(jwatt)
(Assignee)

Comment 53

6 years ago
Created attachment 573125 [details] [diff] [review]
Part 19 - Remove unnecessary serialisation from setting SVGNumberList
Attachment #573125 - Flags: review?(jwatt)
(Assignee)

Comment 54

6 years ago
Created attachment 573126 [details] [diff] [review]
Part 20 - Remove unnecessary serialisation from setting SVGPointList
Attachment #573126 - Flags: review?(jwatt)
(Assignee)

Comment 55

6 years ago
Created attachment 573128 [details] [diff] [review]
Part 21 - Remove unnecessary serialisation from setting SVGPathSegList
Attachment #573128 - Flags: review?(jwatt)
(Assignee)

Comment 56

6 years ago
Created attachment 573129 [details] [diff] [review]
Part 22 - Remove unnecessary serialisation from setting SVGTransformList
Attachment #573129 - Flags: review?(jwatt)
(Assignee)

Comment 57

6 years ago
Created attachment 573131 [details] [diff] [review]
Part 23 - Move redundancy checking from SVG types to event dispatch

This patch is somewhat questionable. A lot of the code in the previous patches deals with checking for and ignoring redundant changes via the DOM APIs. Previously SetParsedAttr used to do this for us by comparing strings before doing anything. Now that we're not serialising though it can't do that for us so we have to do it ourselves.

In the patches up to this point we matched the previous behaviour by adding a lot of redundancy checking to the SVG types. This has the advantage of filtering out unnecessary changes as early as possible.

However, I think the only place this redundancy checking (or lack thereof) is observable from the outside is via mutation events. So another option is to just check the string fields of the mutation event if and when we send one since in that case we will have to serialise the attribute values anyway.

It means we don't filter out redundant changes until much later but it means less code. :) I'm quite ok with not including this patch. Let me know what you think.
Attachment #573131 - Flags: review?(jwatt)
(Assignee)

Comment 58

6 years ago
Created attachment 573132 [details] [diff] [review]
Part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them

There are actually two purposes to this patch:
a) Stop exporting SVG types from the SVG module just for nsAttrValue's sake
b) Tidy up nsAttrValue to remove redundant code when handling the SVG types.

I think (a) is important. However, (b) is perhaps a little hacky in parts but not too bad. Depending on what you think we can alter this patch to just do (a).
Attachment #573132 - Flags: review?(jwatt)
(Assignee)

Comment 59

6 years ago
Created attachment 573133 [details] [diff] [review]
Roll-up patch (patches 1-24) for cross-reference
(Assignee)

Comment 60

6 years ago
That's it for now. I'll have a think about how to split up the review workload a bit since it's a bit rough on Jonathan to land him with 19!
(Assignee)

Updated

6 years ago
No longer blocks: 608495

Comment 61

6 years ago
What about Bug #698996 being a blocker? Does it belong here or somewhere else?
(Assignee)

Comment 62

6 years ago
(In reply to Daniel Roesler from comment #61)
> What about Bug #698996 being a blocker? Does it belong here or somewhere
> else?

Yeah, I just ran a quick test with that one now and, while I don't want to give hard numbers because there's currently a lot of noise on my system, these patches make a big difference. I did a quick profile of that test case too and a lot of the work is in serialising path data (when setting the path segments) which is where these patches help.

Comment 63

6 years ago
Try run for 65abe8c5de1f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=65abe8c5de1f
Results (out of 214 total builds):
    exception: 1
    success: 185
    warnings: 26
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-65abe8c5de1f

Comment 64

6 years ago
Try run for b769712f67f3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b769712f67f3
Results (out of 220 total builds):
    success: 184
    warnings: 32
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-b769712f67f3
(Assignee)

Comment 65

6 years ago
Created attachment 573743 [details] [diff] [review]
Part 2 - Make BeforeSetAttr take an nsAttrValue v2

Bug fix.
Attachment #573108 - Attachment is obsolete: true
Attachment #573743 - Flags: review?(bzbarsky)
Attachment #573108 - Flags: review?(bzbarsky)
(Assignee)

Comment 66

6 years ago
Created attachment 573744 [details] [diff] [review]
Roll-up patch (patches 1-24) for cross-reference v1b

Update roll-up patch with changes to part 2
Attachment #573133 - Attachment is obsolete: true

Comment 67

6 years ago
Try run for 728a619377a7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=728a619377a7
Results (out of 209 total builds):
    success: 189
    warnings: 20
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-728a619377a7
Comment on attachment 573114 [details] [diff] [review]
Part 8 - Remove unnecessary serialisation from setting nsSVGLength2

>+// Helper methods for Will/DidChangeXXX method pairs.
>+//
>+// For some SVG data types, we use an nsAttrValue that just points to an SVG
>+// object (e.g. nsSVGLength2). This saves storing the attribute value twice
>+// (once here in SVG land and once more in the nsAttrValue). However, it means
>+// that once we update the SVG object the old value is lost.
>+//
>+// In the case where we have mutation listeners, we need the old value in order
>+// to set the prevValue member of the mutation event. In order to support this
>+// we require a separate call to WillChangeXXX before changing the SVG class
>+// where we pass back a serialized version of the (soon to be old) attribute
>+// value. If there are no mutation listeners (the common case) we skip this
>+// serialisation altogether and just return an empty attribute value.
>+//
>+// Note that unlike using SetParsedAttr, using Will/DidChangeXXX does NOT check
>+// and filter out redundant changes. Before calling WillChangeXXX, the call site
>+// should check if the value will change and if not, skip calling
>+// Will/DidChangeXXX.
>+nsAttrValue
>+nsSVGElement::WillChangeValue(nsIAtom* aName)
>+{

My understanding is that the code that checks whether an attribute change should trigger a restyle also needs the old attribute value in order to work. Boris should be able to explain that in more detail. Unfortunately, if correct, it would seem that when someone changes an SVG attribute's DOM representation we're either going to have to serialize or create a second internal object tree to represent the old value. Either way, it seems hard to avoid the perf hit. I'll attach a testcase for you to check whether your current patches break restyling.
Created attachment 574159 [details]
test restyles when 'zoomAndPan' attribute changes
> My understanding is that the code that checks whether an attribute change should trigger
> a restyle also needs the old attribute value in order to work.

Yes, though we could talk about trying to change that...

> we're either going to have to serialize or create a second internal object tree to
> represent the old value.

Or call AttributeWillChange _before_ mutating the old value's internal data structure.  That's what @style does, for example.
(Assignee)

Comment 71

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #70)
> Or call AttributeWillChange _before_ mutating the old value's internal data
> structure.  That's what @style does, for example.

Yeah, that's what these patches do too (see part 8 -- nsSVGElement::WillChangeValue). The test case seems to work in my patched build too.
I think my assumptions about what the restyling code does are wrong - I thought that it needs to be able to simultaneously have access to both the old value and the new value to check if the attribute has genuinely changed and, if so, do any necessary restyling. Given that assumption, it's not clear to me how calling anything earlier helps without some way of storing the old/new value separately so they can be compared. Can one of you clarify?
> I thought that it needs to be able to simultaneously have access to both the old value
> and the new value

Ah.  No, it does not need that.  It needs access to the old value in AttributeWillChange and access to the new value in AttributeChanged.
(In reply to Brian Birtles (:birtles) from comment #29)
> (In reply to Robert Longson from comment #28)
> > It looks like this depends on bug 696078 and bug 698195 There are overrides
> > to DidChangeValue in nsSVGFilters that need to go away before you can make
> > them non-virtual. bug 696210 is also in the same area and can only land
> > after bug 696078 and bug 698195), maybe this bug superseeds that though.
> 
> I think bug 696078 is only dealing with changes to DidAnimateXXX? So no
> conflict?
> 
> This patch doesn't touch DidChangeString--it leaves it as virtual so it
> shouldn't be dependent on bug 698195.
> 
> Having a look at bug 696210, that bug's patch and this work overlap
> significantly. About half of the patch for bug 696210 is covered by this
> patch (the missing code being mostly the changes to DidChangeString and
> DidAnimateXXX).
> We can either:
> a) Land bug 696210 as is (after fixing bitrot from bug 698630) and rework
> this bug to fit on top, or
> b) Alter bug 696210 to just deal with the DidAnimateXXX changes, then deal
> with DidChangeString in bug 698195 and the rest of the DidChangeXXX methods
> here. That would save you having to fix most of the bitrot caused by bug
> 698630.
> 

I think b) is the way to go. Can you integrate the DidCahngeXXX parts of bug 696210 that make sense here? I'll rework what's left in bug 696210 later.
(Assignee)

Comment 75

6 years ago
Created attachment 578986 [details] [diff] [review]
Part 2 - Make BeforeSetAttr take an nsAttrValue v3

Fix bitrot.
Attachment #573743 - Attachment is obsolete: true
Attachment #578986 - Flags: review?(bzbarsky)
Attachment #573743 - Flags: review?(bzbarsky)
(Assignee)

Comment 76

6 years ago
Created attachment 578987 [details] [diff] [review]
Part 3 - Make AfterSetAttr take an nsAttrValue v2

Fix bitrot.
Attachment #573109 - Attachment is obsolete: true
Attachment #578987 - Flags: review?(bzbarsky)
Attachment #573109 - Flags: review?(bzbarsky)
(Assignee)

Comment 77

6 years ago
Created attachment 578988 [details] [diff] [review]
Part 8 - Remove unnecessary serialisation from setting nsSVGLength2 v2

Fix bitrot.
Attachment #573114 - Attachment is obsolete: true
Attachment #578988 - Flags: review?(jwatt)
Attachment #573114 - Flags: review?(jwatt)
(Assignee)

Comment 78

6 years ago
Created attachment 578989 [details] [diff] [review]
Part 16 - Add mutation event tests for strings and classes and tidy up DidChangeString usage v2
Attachment #573122 - Attachment is obsolete: true
Attachment #578989 - Flags: review?
Attachment #573122 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578989 - Flags: review? → review?(jwatt)
(Assignee)

Comment 79

6 years ago
Created attachment 578990 [details] [diff] [review]
Part 17 - Remove unnecessary serialisation from setting SVGPreserveAspectRatio v2
Attachment #573123 - Attachment is obsolete: true
Attachment #578990 - Flags: review?(jwatt)
Attachment #573123 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578989 - Attachment description: Add mutation event tests for strings and classes and tidy up DidChangeString usage v2 → Part 16 Add mutation event tests for strings and classes and tidy up DidChangeString usage v2
(Assignee)

Updated

6 years ago
Attachment #578989 - Attachment description: Part 16 Add mutation event tests for strings and classes and tidy up DidChangeString usage v2 → Part 16 - Add mutation event tests for strings and classes and tidy up DidChangeString usage v2
(Assignee)

Comment 80

6 years ago
Created attachment 578991 [details] [diff] [review]
Part 18 - Remove unnecessary serialisation from setting nsSVGViewBox v2
Attachment #573124 - Attachment is obsolete: true
Attachment #578991 - Flags: review?(jwatt)
Attachment #573124 - Flags: review?(jwatt)
(Assignee)

Comment 81

6 years ago
Created attachment 578992 [details] [diff] [review]
Part 19 - Remove unnecessary serialisation from setting SVGNumberList v2
Attachment #573125 - Attachment is obsolete: true
Attachment #578992 - Flags: review?(jwatt)
Attachment #573125 - Flags: review?(jwatt)
(Assignee)

Comment 82

6 years ago
Created attachment 578993 [details] [diff] [review]
Part 20 - Remove unnecessary serialisation from setting SVGPointList v2
Attachment #573126 - Attachment is obsolete: true
Attachment #578993 - Flags: review?(jwatt)
Attachment #573126 - Flags: review?(jwatt)
(Assignee)

Comment 83

6 years ago
Created attachment 578994 [details] [diff] [review]
Part 21 - Remove unnecessary serialisation from setting SVGPathSegList v2
Attachment #573128 - Attachment is obsolete: true
Attachment #578994 - Flags: review?(jwatt)
Attachment #573128 - Flags: review?(jwatt)
(Assignee)

Comment 84

6 years ago
Created attachment 578995 [details] [diff] [review]
Part 22 - Remove unnecessary serialisation from setting SVGTransformList v2
Attachment #573129 - Attachment is obsolete: true
Attachment #578995 - Flags: review?(jwatt)
Attachment #573129 - Flags: review?(jwatt)
(Assignee)

Comment 85

6 years ago
(In reply to Robert Longson from comment #74)
> I think b) is the way to go. Can you integrate the DidCahngeXXX parts of bug
> 696210 that make sense here? I'll rework what's left in bug 696210 later.

Done. Basically the only thing which was missing was a couple of tweaks to DidChangeString which I've added to patch 16 here. I've gone through all the other code in bug 696210 and it's pretty much all included in these patches which the exception of making the DidAnimateXXX stuff non-virtual.
(Assignee)

Comment 86

6 years ago
Boris and Jonathan is there anything I can do to help with getting these patches reviewed? It's been nearly a month now and I was hoping to get this onto the next release train.

Jonathan, I realise I've asked you to review a lot of patches. I didn't divide the work up further because those patches are all very similar. Once you've reviewed the first three patches or so, you can probably review the others very quickly. But if it still seems overwhelming perhaps I can ask Robert Longson to look at half of them?
(Assignee)

Comment 87

6 years ago
Created attachment 578998 [details] [diff] [review]
Roll-up patch (patches 1-24) for cross-reference v1c
Attachment #573744 - Attachment is obsolete: true
Sorry Brian, I should have said, but I was waiting to see that Boris was happy with you contents changes before reviewing how you use those changes in SVG. Boris, do you think you'll able to look at this any time soon?
There's also bug 607854 which adds the one missing type from SVG - string lists We'll need to integrate this change with that one at some point.
> Boris, do you think you'll able to look at this any time soon?

Hopefully toward the end of this week.....
bug 607854 has landed so can you convert the new StringList type Brian?
I'm sorry for the lag here; I've been spending way more time than I expected on some other things, and then holidays happened... :(

I'm not likely to get to this before Tuesday.  I do hope to look at it at that point, but if that doesn't happen it'll be end of January.

If Jonas can review before those timeframes (which I sort of doubt), it might be worth asking him instead.

Again, I'm really sorry for the lag.
(Assignee)

Comment 93

6 years ago
Created attachment 592563 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings

Fix bitrot
Attachment #592563 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #573107 - Attachment is obsolete: true
Attachment #573107 - Flags: review?(bzbarsky)
(Assignee)

Comment 94

6 years ago
Created attachment 592564 [details] [diff] [review]
part 2 - Make BeforeSetAttr take an nsAttrValue

Fix bitrot
Attachment #592564 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #578986 - Attachment is obsolete: true
Attachment #578986 - Flags: review?(bzbarsky)
(Assignee)

Comment 95

6 years ago
Created attachment 592566 [details] [diff] [review]
part 3 - Make AfterSetAttr take an nsAttrValue

Fix bitrot
Attachment #592566 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #578987 - Attachment is obsolete: true
Attachment #578987 - Flags: review?(bzbarsky)
(Assignee)

Comment 96

6 years ago
Created attachment 592567 [details] [diff] [review]
part 4 - Tidy up bool parameters to make them easier to read

Fix bitrot
Attachment #592567 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #573110 - Attachment is obsolete: true
Attachment #573110 - Flags: review?(bzbarsky)
(Assignee)

Comment 97

6 years ago
Created attachment 592568 [details] [diff] [review]
part 5 - Add assignment operator to nsAttrValue

Fix bitrot
Attachment #592568 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #573111 - Attachment is obsolete: true
Attachment #573111 - Flags: review?(bzbarsky)
(Assignee)

Comment 98

6 years ago
Created attachment 592569 [details] [diff] [review]
part 6 - Add test framework for modification events

Fix bitrot
Attachment #592569 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573112 - Attachment is obsolete: true
Attachment #573112 - Flags: review?(jwatt)
(Assignee)

Comment 99

6 years ago
Created attachment 592570 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum

Fix bitrot
Attachment #592570 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573113 - Attachment is obsolete: true
Attachment #573113 - Flags: review?(jwatt)
(Assignee)

Comment 100

6 years ago
Created attachment 592571 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

Fix bitrot
Attachment #592571 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578988 - Attachment is obsolete: true
Attachment #578988 - Flags: review?(jwatt)
(Assignee)

Comment 101

6 years ago
Created attachment 592572 [details] [diff] [review]
part 9 - Update attribute setting for SVGAnimatedLengthList

Fix bitrot
Attachment #592572 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573115 - Attachment is obsolete: true
Attachment #573115 - Flags: review?(jwatt)
(Assignee)

Comment 102

6 years ago
Created attachment 592573 [details] [diff] [review]
part 10 - Remove unnecessary serialisation from setting nsSVGNumber2

Fix bitrot
Attachment #592573 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573116 - Attachment is obsolete: true
Attachment #573116 - Flags: review?(jwatt)
(Assignee)

Comment 103

6 years ago
Created attachment 592574 [details] [diff] [review]
part 11 - Remove unnecessary serialisation from setting nsSVGNumberPair

Fix bitrot
Attachment #592574 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573117 - Attachment is obsolete: true
Attachment #573117 - Flags: review?(jwatt)
(Assignee)

Comment 104

6 years ago
Created attachment 592575 [details] [diff] [review]
part 12 - Remove unnecessary serialisation from setting nsSVGInteger

Fix bitrot
Attachment #592575 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573118 - Attachment is obsolete: true
Attachment #573118 - Flags: review?(jwatt)
(Assignee)

Comment 105

6 years ago
Created attachment 592576 [details] [diff] [review]
part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair

Fix bitrot
Attachment #592576 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573119 - Attachment is obsolete: true
Attachment #573119 - Flags: review?(jwatt)
(Assignee)

Comment 106

6 years ago
Created attachment 592578 [details] [diff] [review]
part 14 - Remove unnecessary serialisation from setting nsSVGAngle

Fix bitrot
Attachment #592578 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573120 - Attachment is obsolete: true
Attachment #573120 - Flags: review?(jwatt)
(Assignee)

Comment 107

6 years ago
Created attachment 592579 [details] [diff] [review]
part 15 - Remove unnecessary serialisation from setting nsSVGBoolean

Fix bitrot
Attachment #592579 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573121 - Attachment is obsolete: true
Attachment #573121 - Flags: review?(jwatt)
(Assignee)

Comment 108

6 years ago
Created attachment 592580 [details] [diff] [review]
part 16 - Add mutation event tests for strings and classes and tidy up use of DidChangeString

Fix bitrot
Attachment #592580 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578989 - Attachment is obsolete: true
Attachment #578989 - Flags: review?(jwatt)
(Assignee)

Comment 109

6 years ago
Created attachment 592581 [details] [diff] [review]
part 17 - Remove unnecessary serialisation from setting nsSVGPreserveAspectRatio

Fix bitrot
Attachment #592581 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578990 - Attachment is obsolete: true
Attachment #578990 - Flags: review?(jwatt)
(Assignee)

Comment 110

6 years ago
Created attachment 592582 [details] [diff] [review]
part 18 - Remove unnecessary serialisation from setting nsSVGViewBox

Fix bitrot
Attachment #592582 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578991 - Attachment is obsolete: true
Attachment #578991 - Flags: review?(jwatt)
(Assignee)

Comment 111

6 years ago
Created attachment 592583 [details] [diff] [review]
part 19 - Remove unnecessary serialisation from setting nsSVGNumberList

Fix bitrot
Attachment #592583 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578992 - Attachment is obsolete: true
Attachment #578992 - Flags: review?(jwatt)
(Assignee)

Comment 112

6 years ago
Created attachment 592584 [details] [diff] [review]
part 20 - Remove unnecessary serialisation from setting SVGPointList

Fix bitrot
Attachment #592584 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578993 - Attachment is obsolete: true
Attachment #578993 - Flags: review?(jwatt)
(Assignee)

Comment 113

6 years ago
Created attachment 592585 [details] [diff] [review]
part 21 - Remove unnecessary serialisation from setting SVGPathSegList

Fix bitrot
Attachment #592585 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578994 - Attachment is obsolete: true
Attachment #578994 - Flags: review?(jwatt)
(Assignee)

Comment 114

6 years ago
Created attachment 592586 [details] [diff] [review]
part 22 - Remove unnecessary serialisation from setting SVGTransformList

Fix bitrot
Attachment #592586 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #578995 - Attachment is obsolete: true
Attachment #578995 - Flags: review?(jwatt)
(Assignee)

Comment 115

6 years ago
Created attachment 592587 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch

Fix bitrot
Attachment #592587 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573131 - Attachment is obsolete: true
Attachment #573131 - Flags: review?(jwatt)
(Assignee)

Comment 116

6 years ago
Created attachment 592588 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them

Fix bitrot
Attachment #592588 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #573132 - Attachment is obsolete: true
Attachment #573132 - Flags: review?(jwatt)
(Assignee)

Comment 117

6 years ago
Patches should now be up to date. I've yet to convert StringList as suggested in comment 91 but that patch should just sit on top of the others.

Next week it will be 3 months since I first requested review. I'd like to get the review process moving since this is an important perf win. Boris, perhaps you could suggest someone else who is suitable for reviewing this? Thanks!
For this code it's probably me or sicking.

For what it's worth, I was planning to make some progress on this review this week.  I just needed to free up a few days in a row to work on it...
(Assignee)

Comment 119

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #118)
> For this code it's probably me or sicking.
> 
> For what it's worth, I was planning to make some progress on this review
> this week.  I just needed to free up a few days in a row to work on it...

Ok, that'd be great. Thanks Boris!
(Assignee)

Comment 120

6 years ago
Created attachment 592607 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them

Remove forward declarations from nsAttrValue.h and just replace with SVGAttrValueWrapper.h which contains them anyway
Attachment #592607 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #592588 - Attachment is obsolete: true
Attachment #592588 - Flags: review?(jwatt)
(Assignee)

Comment 121

6 years ago
Created attachment 592609 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList

Incorporate SVGStringList too
Attachment #592609 - Flags: review?(jwatt)

Comment 122

6 years ago
Try run for e96c921bf6d9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e96c921bf6d9
Results (out of 208 total builds):
    success: 188
    warnings: 20
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-e96c921bf6d9
(Assignee)

Comment 123

6 years ago
Comment on attachment 592609 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList

Forgot to include serialisation. Updated patch forthcoming.
Attachment #592609 - Attachment is obsolete: true
Attachment #592609 - Flags: review?(jwatt)
(Assignee)

Comment 124

6 years ago
Created attachment 592619 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList

Updated SVGStringList patch. I had to move the "isCommaSeparated" flag into the SVGStringList class since nsAttrValue objects need to know how to serialise themselves.
Attachment #592619 - Flags: review?(jwatt)
(Assignee)

Comment 125

6 years ago
Created attachment 592951 [details] [diff] [review]
part 4 - Tidy up bool parameters to make them easier to read

Fix bitrot
Attachment #592951 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #592567 - Attachment is obsolete: true
Attachment #592567 - Flags: review?(bzbarsky)
Comment on attachment 592570 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum

> nsSVGElement::ParseAttribute(PRInt32 aNamespaceID,
>                              nsIAtom* aAttribute,
>                              const nsAString& aValue,
>                              nsAttrValue& aResult)
> {
>   nsresult rv = NS_OK;
>   bool foundMatch = false;
>+  bool setResult = false;

This sounds like the imperative. Can you change it to didSetResult? As an extra patch tacked onto the end of your patch queue is fine, so you don't need to juggle your patches.


>+nsIAtom*
>+nsSVGEnum::GetBaseValueAtom(nsSVGElement *aSVGElement)
> {
...
>   }
>   NS_ERROR("unknown enumeration value");
>+  return nsnull;

Seems safer to return nsGkAtoms::_empty here.
Attachment #592570 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Duplicate of this bug: 609527
Comment on attachment 592563 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings

>+nsGenericElement::MaybeCheckSameAttrVal(PRInt32 aNamespaceID,
>+        // Need to store the old value.
>+        // If the current attribute value contains a pointer to some other data
>+        // structure that gets updated in the process of setting the attribute
>+        // we'll no longer have the old value of attribute.

"of the attribute"

>+        if (info.mValue->Type() != nsAttrValue::eString &&
>+            info.mValue->Type() != nsAttrValue::eAtom) {
>+          nsAutoString serializedValue;
>+          info.mValue->ToString(serializedValue);
>+          aOldValue.SetTo(serializedValue);
>+        } else {
>+          aOldValue.SetTo(*info.mValue);
>+        }

Hmm.  This is a bit of a pain, but I guess we're not going to need this anywhere else so there's no point moving it into an nsAttrValue method?

>+      // Compare the values but remember that both aValue (the new value) and
>+      // the existing value may a parsed value or a regular string value.

"may be a parsed value"

>+      // First, try a typed comparison:

This part I _definitely_ think we should move into nsAttrValue.  Maybe call it LooselyEquals or EqualsAsStrings or something?  There's the confusion that we have both *info.mValue and aOldValue, I guess...

In any case, this should be factored out as a separate method.  The code as written is _really_ hard to follow, and seems to be relying on various nsAttrValue internal details.  The weird indentation on the comments doesn't help....

> nsGenericElement::SetAttrAndNotify(PRInt32 aNamespaceID,
>+    if (!aOldValue.IsEmptyString()) {
>+      switch (aOldValue.Type()) {
....

This whole chunk with the switch and such should become an |already_AddRefed<nsIAtom> nsAttrValue::GetAsAtom() const| method.  That way we don't have to worry about attr value guts here.

>+++ b/content/base/src/nsGenericElement.h
>   bool MaybeCheckSameAttrVal(PRInt32 aNamespaceID, nsIAtom* aName,

aOldValue is always set as a string, right?  Will that continue being the case?  Might be worth documenting exactly what one can expect or not out of aOldValue.

I'd like to see an updated version of this patch with the refactoring bits, but modulo the nits above this looks good.
Attachment #592563 - Flags: review?(bzbarsky) → review+
Comment on attachment 592564 [details] [diff] [review]
part 2 - Make BeforeSetAttr take an nsAttrValue

r=me
Attachment #592564 - Flags: review?(bzbarsky) → review+
Comment on attachment 592566 [details] [diff] [review]
part 3 - Make AfterSetAttr take an nsAttrValue

In nsGenericHTMLFormElement::AfterSetAttr for the name/id case we know we have an atom value, right?  You should be able to use nsDependentAtomString instead of having to ToString the value, I think.

For the nsXULElement::AfterSetAttr case, we only have to check Type() because it can be an atom, right?  nsAttrValue could easily return an nsCheapString in the atom case too.  Could you please file a followup bug on adding a way to do that and making the XUL code use that?

>+              HideWindowChrome(aValue->Equals(NS_LITERAL_STRING("true"),
>+                               eCaseMatters));

Weird indent here.  Please fix.

r=me with that
Attachment #592566 - Flags: review?(bzbarsky) → review+
Comment on attachment 592951 [details] [diff] [review]
part 4 - Tidy up bool parameters to make them easier to read

r=me
Attachment #592951 - Flags: review?(bzbarsky) → review+
Comment on attachment 592568 [details] [diff] [review]
part 5 - Add assignment operator to nsAttrValue

r=me

I'm really sorry this took so long.  I recall starting to read this patch queue and feeling like it was really scary and complicated, but this time around it didn't seem that bad.... :(

Next time please poke me earlier?  ;)
Attachment #592568 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 133

6 years ago
Created attachment 594643 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz

Update review feedback
(Assignee)

Updated

6 years ago
Attachment #592563 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #594643 - Attachment description: part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; → part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz
Attachment #594643 - Flags: review+
(Assignee)

Comment 134

6 years ago
Created attachment 594644 [details] [diff] [review]
part 3 - Make AfterSetAttr take an nsAttrValue; r=bz

Address review feedback.
Attachment #592566 - Attachment is obsolete: true
Attachment #594644 - Flags: review+
(Assignee)

Updated

6 years ago
Blocks: 724466
(Assignee)

Comment 135

6 years ago
Created attachment 594645 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum; r=jwatt

Address review feedback.
Attachment #592570 - Attachment is obsolete: true
Attachment #594645 - Flags: review+
(Assignee)

Comment 136

6 years ago
Created attachment 594646 [details] [diff] [review]
Roll-up patch (patches 1-25) for cross-reference v1d

Updated roll-up patch for cross reference.
Attachment #578998 - Attachment is obsolete: true
(Assignee)

Comment 137

6 years ago
Thanks Boris! Much appreciated. The first patch looks a lot better with your feedback incorporated. Please give it a glance and check that it matches what you had in mind.

Thanks Jonathan as well! I went ahead and updated the patches directly instead of adding another patch on top since I was touching them all anyway to rebase on top of the changes from Boris' reviews. To avoid bug spam, I haven't bothered posting all the updated patches however--I'll wait until I get your feedback before updating them here. In the mean time, that means the current queue won't compile but I've posted an updated roll-up patch that should.

I'm going to go ahead and send patches 1-5 to Try. Depending on how long you think it will take you Jonathan, it might be worth landing those patches first to avoid more bitrot.
(Assignee)

Comment 138

6 years ago
Created attachment 594650 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList v1b

Fix missing call to set the attribute using the parsed attribute interface.
Attachment #592619 - Attachment is obsolete: true
Attachment #594650 - Flags: review?(jwatt)
Attachment #592619 - Flags: review?(jwatt)

Comment 139

6 years ago
Try run for d5f951fa96a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d5f951fa96a5
Results (out of 273 total builds):
    success: 204
    warnings: 48
    failure: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-d5f951fa96a5
(In reply to Brian Birtles (:birtles) from comment #137)
> it might be worth landing those patches first to avoid more bitrot.

Please do, although I'm going to be working on getting through these patches as soon as possible.
My main concern about these patches is whether it's safe for nsAttrValue objects to have raw, non-owning pointers into SVG element objects. It's not entirely clear to me whether element objects always outlive all nsAttrValue objects created for them.

Boris, presumably you considered that during your reviews and you're fine with that?
> Boris, presumably you considered that during your reviews and you're fine with that?

I didn't consider it, since the patches I looked at didn't do anything like that.

That said, I believe this is safe.  nsAttrValue objects can basically live in three places:

1)  On the stack.  This should be safe, right?
2)  In the element's attr and child array (as values, not as pointers!).  This is
    obviously safe if the element keeps the things being pointed to alive.
3)  In the mapped attributes.  As long as none of the relevant SVG attributes are mapped,
    this is a non-issue.  That should be double-checked, and perhaps asserted somewhere?
    Maybe in whatever ParseAttribute functions create such attr values?

> Please give it a glance and check that it matches what you had in mind.

Looks pretty good.  Please document the new nsAttrValue methods, though.

Also, in part 3 in nsGenericHTMLFormElement::AfterSetAttr the NS_ABORT_IF_FALSE seems to be backwards, no?  The value should be an atom value there...
Aren't outer svg element width and height values mapped? http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#885
> Aren't outer svg element width and height values mapped

Yes, they are.  OK, then we have to think about it a bit more.

So in general mapped attributes are stored in a refcounted nsMappedAttributes data structure.  The nsAttrAndChildArray holds a reference to this object.  These objects _can_ be shared across multiple attr and child arrays.  The way this works is that whenever an nsMappedAttributes is modified we take the post-modification version and see whether we have an existing object equal to it (in the nsMappedAttributes::Equals sense).  If so, we drop the just-modified object and point to the existing one.  Note that this involves Equals() testing true on the corresponding nsAttrValues in the two lists.  So as long as nsAttrValue::Equals uses pointer equality for the SVG stuff there should be no cross-element coalescing of mapped attributes that hold the SVG objects, and then the element's nsAttrAndChildArray will hold the unique ref to the nsMappedAttributes and everything should be fine.  Does mean the assert suggestion won't work. ;)
(In reply to Boris Zbarsky (:bz) from comment #144)
> nsMappedAttributes and everything should be fine.  Does mean the assert
> suggestion won't work. ;)

Yes, I don't think it will work unless you do something special for those two attributes.
(Reporter)

Updated

6 years ago
Duplicate of this bug: 340835
(In reply to Boris Zbarsky (:bz) from comment #144)
> So in general mapped attributes are stored in a refcounted
> nsMappedAttributes data structure.

Just a note - that's not the case currently for the SVG code (we don't use nsMappedAttributes), but it very possibly will be at some point in the future (if we push the mapped attribute stuff from nsGenericHTMLElement up to nsGenericElement). The patches here should be written to assume that will happen, I think (in terms of adding asserts).

> The nsAttrAndChildArray holds a
> reference to this object.  These objects _can_ be shared across multiple
> attr and child arrays.

This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls, right? Probably we should not call that for SVG elements if/when we push the mapped attr stuff up to nsGenericElement...?

> as long as
> nsAttrValue::Equals uses pointer equality for the SVG stuff

Hmm, it doesn't seem like we should do pointer comparison there, except as a return-true shortcut. That was actually one of my upcoming comments on this patch series.

> Does mean the assert suggestion won't work.

I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment, then if we move the mapped attr stuff up to nsGenericElement that asserts will remind us to make sure nsMappedAttributes objects are not shared between SVG elements.
> we don't use nsMappedAttributes

You don't?  You're returning true from IsAttributeMapped(), no?  nsGenericElement uses nsMappedAttributes in that case.  Or am I missing something.

> This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls,
> right?

Yes.  Skipping calling it might make sense if it'll always be a no-op for SVG.  But then why would you want to use nsMappedAttributes at all?  The whole point of that is sharing across elements; if you don't plan to use it, you could just use some other simple implementation for SVG.

> Hmm, it doesn't seem like we should do pointer comparison there

OK.  Then we need to be very careful about mapped attributes.

> I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check
> |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment

Does such an assert not assert on trunk right now?
(In reply to Boris Zbarsky (:bz) from comment #148)
> > we don't use nsMappedAttributes
> 
> You don't?  You're returning true from IsAttributeMapped(), no? 
> nsGenericElement uses nsMappedAttributes in that case.  Or am I missing
> something.

We return true from IsAttributeMapped, but SVG elements don't inherit nsMappedAttributeElement, so the SetMappedAttribute call invokes nsGenericElement::SetMappedAttribute which returns false.

> > This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls,
> > right?
> 
> Yes.  Skipping calling it might make sense if it'll always be a no-op for
> SVG.  But then why would you want to use nsMappedAttributes at all?

So that the SVG code can share the code that gets the nsIStyleRule from nsMappedAttributes, and use the MapRuleInfoInto from that, rather than having it's own custom code for mapping attributes into style?

> The
> whole point of that is sharing across elements; if you don't plan to use it,
> you could just use some other simple implementation for SVG.

Ok, maybe we can just keep the SVG implementation if it's not really a win to share the normal attr mapping code.

> > Hmm, it doesn't seem like we should do pointer comparison there
> 
> OK.  Then we need to be very careful about mapped attributes.

Yup. Would the following assert do...?

> > I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check
> > |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment
> 
> Does such an assert not assert on trunk right now?

Not that I'm aware of.
Of course nsAttrAndChildArray::MappedAttrCount() is private - is it ok to make that public, bz?
> but SVG elements don't inherit nsMappedAttributeElement, so the SetMappedAttribute call
> invokes nsGenericElement::SetMappedAttribute

Ah, right.  We have too many element classes.  :(

> So that the SVG code can share the code that gets the nsIStyleRule from
> nsMappedAttributes, and use the MapRuleInfoInto from that, rather than having it's own
> custom code for mapping attributes into style?

OK... I'd have to dig through a bit to see whether you'd really be sharing all that much code once you're not sharing a bunch of the real guts (which is the coalescing across elements).  In particular, if the element could have a direct pointer to the style rule (which sounds like what you'd be doing in the SVG case), then bloating the mapped attribute hashtable with all those singleton rules may not be a very good idea.  I agree that we'd want to restructure the code somehow to make you not have to reinvent wheels, though.

> Yup. Would the following assert do...?

Yes.  Though make it HasMappedAttributes() for the public method, since that's what you really want to know.
> > The
> > whole point of that is sharing across elements; if you don't plan to use it,
> > you could just use some other simple implementation for SVG.
> 
> Ok, maybe we can just keep the SVG implementation if it's not really a win
> to share the normal attr mapping code.

To be clear, the main point of the sharing is so that all elements with the same set of style mapping attributes share the same nsIStyleRule object.

This is important due to some details of how the style-rule-tree works since otherwise you can end up with an explosion of style-rule-tree branches due to the fact that the nsIStyleRules for mapped attributes have very low precedence.

At least that is my understanding. Though that might have changed over the years and is no longer the case, or is no longer as important.
It's still important for HTML (think a page with a bajillion of <font> tags with the same "face" attribute).  The key questions are:

1)  Would this sort of sharing be useful for SVG?  That is would it be common to have lots of elements with identical style-mapped attributes on them?  I'd guess no.

2)  Given that the sharing is not useful for SVG, how could/should we refactor the mapped attr code so that we have minimal code duplication but don't do all the attempted sharing stuff, sorting, storage in a separate object with resulting mis-ordering of attributes, etc, for SVG?
I've spun off bug 724680 to discuss sharing more of the mapped attribute code to keep down the noise here. Can you add your comments there?
Brian, I think for you the only actionable point since comment 141 is:

Please add a public HasMappedAttributes() method to nsAttrAndChildArray, then add an inline nsSVGElement::AfterSetAttr with an NS_ABORT_IF_FALSE that checks that it always returns false for SVG elements. Also add a nice detailed comment there explaining why we need to be very careful if we start using nsMappedAttributes for SVG elements (i.e. some nsAttrValues point to member data of SVG elements, and we don't want to leave dangling pointers). Probably have the comment point to bug 724680 too.
(In reply to Jonathan Watt [:jwatt] from comment #155)
> then add an inline nsSVGElement::AfterSetAttr with...

Sorry, it's BeforeSetAttr that doesn't currently exist - obviously just add the assert and comment to the existing nsSVGElement::AfterSetAttr.
Comment on attachment 592571 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

Review of attachment 592571 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrValue.cpp
@@ +709,5 @@
>        return thisCont->mIntMargin == otherCont->mIntMargin;
>      }
> +    case eSVGLength:
> +    {
> +      return thisCont->mSVGLength == otherCont->mSVGLength;

We should be doing object comparison here, not pointer comparison. And we want to only compare the baseVal members. You could add a BaseValEquals() method to nsSVGLength2 (and the other types in the patch series) just for that purpose (rather than operator==).

That said, it would still be good to have a pointer comparison short circuit to save us having to compare objects (especially some of the larger object types such as lists) if the two objects are the same object. That check would probably be best off in the BaseValEquals() (or whatever you call it) method though, rather than here.

::: content/svg/content/src/nsSVGElement.cpp
@@ +1256,5 @@
>                                               SMIL_MAPPED_ATTR_STYLERULE_ATOM,
>                                               nsnull));
>  }
>  
> +// Helper methods for Will/DidChangeXXX method pairs.

Can you use a Doxygen style comment, and make the comment:

/**
 * Helper methods for the type-specific WillChangeXXX methods.
 *
 * This method sends out appropriate pre-change notifications so that selector
 * restyles (e.g. due to changes that cause |elem[attr="val"]| to start/stop
 * matching) work, and it returns an nsAttrValue that _may_ contain the
 * attribute's pre-change value.
 *
 * The nsAttrValue returned by this method depends on whether there are
 * mutation event listeners listening for changes to this element's attributes.
 * If not, then the object returned in empty. If there are, then the
 * nsAttrValue returned contains a serialized copy of the attribute's value
 * prior to the change, and this object _MUST_ be passed to the corresponding
 * DidChangeXXX method call. This is necessary so that the 'prevValue' property
 * of the mutation event that is dispatched will correctly contain the old
 * value.
 *
 * The reason we need to serialize the old value if there are mutation
 * event listeners is because the underlying nsAttrValue for the attribute
 * points directly to a parsed representation of the attribute (e.g. an
 * SVGAnimatedLengthList*) that is a member of the SVG element. That object
 * will have changed by the time DidChangeXXX has been called, so without the
 * serialization of the old attribute value that we provide, DidChangeXXX
 * would have no way to get the old value to pass to SetAttrAndNotify.
 *
 * We only return the old value when there are mutation event listeners because
 * it's not needed otherwise, and because it's expensive to serialize the old
 * value. This is especially true for list type attributes, which may be built
 * up via the SVG DOM resulting in a large number of Will/DidModifyXXX calls
 * before the script finally finishes setting the attribute.
 *
 * Note that unlike using SetParsedAttr, using Will/DidChangeXXX does NOT check
 * and filter out redundant changes. Before calling WillChangeXXX, the caller
 * should check whether the new and old values are actually the same, and skip
 * calling Will/DidChangeXXX if they are.
 */

@@ +1289,5 @@
> +
> +  // This is not strictly correct--the attribute value parameter for
> +  // BeforeSetAttr should reflect the value that *will* be set but that implies
> +  // allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
> +  // since no SVG elements overload BeforeSetAttr. For now we just pass the

Probably worth adding a debug only BeforeSetAttr to nsSVGElement that defers to nsSVGElementBase, and is marked using MOZ_FINAL, with a comment flagging this issue.

@@ +1320,5 @@
> +  return emptyOrOldAttrValue;
> +}
> +
> +void
> +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,

It seems like aOldValue should be aEmptyOrOldValue.

Again, a Doxygen style comment would be good:

/**
 * Helper methods for the type-specific DidChangeXXX methods.
 *
 * aEmptyOrOldValue must be the object returned from the corresponding
 * WillChangeXXX call. If we have mutation event listeners things will go
 * wrong if it isn't.
 */

@@ +1322,5 @@
> +
> +void
> +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,
> +                             nsAttrValue& aNewValue)
> +{

It would be good if we could have an NS_ABORT_IF_FALSE below checking that if we have mutation listeners, aEmptyOrOldValue is not uninitialized. I'm not really sure how you can do that currently though, since the best nsAttrValue seems to offer is IsEmptyString(), which is not what we want. Maybe we can add a bogus "not initialized" type to nsAttrValue purely for this purpose? Boris/Jonas?

@@ +1440,5 @@
> +  return WillChangeValue(*GetLengthInfo().mLengthInfo[aAttrEnum].mName);
> +}
> +
> +void
> +nsSVGElement::DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aOldValue)

And again here - aEmptyOrOldValue. And in fact for all DidChangeXXX definitions/declarations.

::: content/svg/content/src/nsSVGLength2.cpp
@@ +313,5 @@
> +  if (mIsBaseSet && mBaseVal == aValue) {
> +    return;
> +  }
> +
> +  nsAttrValue oldValue;

Can you rename oldValue to emptyOrOldValue, since it's only the old value if we have mutation listeners, right? Same goes for the other places where WillChangeLength (and indeed all other WillChangeXXX methods) are called.

@@ +338,5 @@
>    if (!IsValidUnitType(unitType))
>      return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>  
> +  if (mSpecifiedUnitType == PRUint8(unitType))
> +    return NS_OK;

I think we want to check mIsBaseSet here too, and send out notifications if it's false. If the default value of the length is 100%, say, and then someone calls convertToSpecifiedUnits() to set it to percentage units, then it seems that the attribute value should go from "" to "100%", even if there's no change to the underlying value. If that's the case, then we should send a mutation event. Probably we need a comment here to explain this as well.

@@ +340,5 @@
>  
> +  if (mSpecifiedUnitType == PRUint8(unitType))
> +    return NS_OK;
> +
> +  nsAttrValue oldValue = aSVGElement->WillChangeLength(mAttrEnum);

Before this line, can you add:

  // Even though we're not changing the visual effect this length will have
  // on the document, we still need to send out notifications in case we have
  // mutation listeners, since the actual string value of the attribute will
  // change.

@@ +346,4 @@
>    float valueInUserUnits = 
>      mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);
>    mSpecifiedUnitType = PRUint8(unitType);
> +  SetBaseValue(valueInUserUnits, aSVGElement, false);

Hmm, under SetBaseValue() we call SetBaseValueInSpecifiedUnits(), so we'll get two Will/DidModifyLength calls for each ConvertToSpecifiedUnits() call...
Comment on attachment 592587 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch

I'm not sure we want to slow down that path for non-SVG elements.
Attachment #592587 - Flags: review?(bzbarsky)
Plus we skip more work if we keep the redundancy checks in the SVG code.
The single atom compare on what's already a slow path is not a big deal for the non-SVG case.  So I'd just worry about how the SVG code wants stuff to work.
Attachment #592587 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 161

6 years ago
Try is looking pretty good so I've pushed parts 1 to 5 to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25edb41d0119
https://hg.mozilla.org/integration/mozilla-inbound/rev/415f3b807d45
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb8a322cbe3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22a4f1815c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/683c21514e28
(Assignee)

Updated

6 years ago
Whiteboard: [inbound, parts 1 to 5 only, don't close yet please]
(Assignee)

Updated

6 years ago
Attachment #592564 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #592568 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #594643 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #594644 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #592951 - Flags: checkin+
(Assignee)

Comment 162

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #142)
> Looks pretty good.  Please document the new nsAttrValue methods, though.

Oops, I missed this comment. Will post a follow-up patch soon.

> Also, in part 3 in nsGenericHTMLFormElement::AfterSetAttr the
> NS_ABORT_IF_FALSE seems to be backwards, no?  The value should be an atom
> value there...

That would explain why the previous try run was so colourful. :) Fixed in the version pushed to m-i.
(Assignee)

Comment 163

6 years ago
Created attachment 594927 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them, v1b

(In reply to Jonathan Watt [:jwatt] from comment #157)
> Comment on attachment 592571 [details] [diff] [review]
> part 8 - Remove unnecessary serialisation from setting nsSVGLength2
> 
> Review of attachment 592571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsAttrValue.cpp
> @@ +709,5 @@
> >        return thisCont->mIntMargin == otherCont->mIntMargin;
> >      }
> > +    case eSVGLength:
> > +    {
> > +      return thisCont->mSVGLength == otherCont->mSVGLength;
> 
> We should be doing object comparison here, not pointer comparison. And we
> want to only compare the baseVal members. You could add a BaseValEquals()
> method to nsSVGLength2 (and the other types in the patch series) just for
> that purpose (rather than operator==).

Just out of curiousity why do we want to do object comparison?

Anyway, my guess is we're not using this for SVG types anyway. I added a breakpoint there and ran the SVG reftests and mochitests and it never got it. I'm now running the same assertion through Try to see if it picks up anything: https://tbpl.mozilla.org/?tree=Try&rev=a9e7362913ef

(By the way, do we have any tools for finding call sites--like Eclipse does with Java. MXR is not particularly helpful for searching for call sites of "Equals".)

Here is an updated patch which replaces the pointer comparison with an NS_ABORT_IF_FALSE(false). I'm reluctant to write all those comparison functions if I can't test them (and it's not just lengths we need to compare, but lists too, and lists of complex stuff--it could end up being quite a bit of code).
Attachment #592607 - Attachment is obsolete: true
Attachment #592607 - Flags: review?(jwatt)
Attachment #594927 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #594927 - Flags: review? → review?(jwatt)

Comment 164

6 years ago
Try run for 5e9beb968c53 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5e9beb968c53
Results (out of 272 total builds):
    success: 251
    warnings: 20
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-5e9beb968c53
(In reply to Brian Birtles (:birtles) from comment #163)
> Just out of curiousity why do we want to do object comparison?

Because nsAttrValue::Equals() is supposed to check if two attributes are equal.

> Anyway, my guess is we're not using this for SVG types anyway.

Maybe not, but it seems like the code should still be correct so that if we in future things work as expected.

> (By the way, do we have any tools for finding call sites--like Eclipse does
> with Java. MXR is not particularly helpful for searching for call sites of
> "Equals".)

DXR is supposed to let you do that, but it seems to be broken right now:

http://dxr.lanedo.com/search.cgi?tree=mozilla-central&callers=nsAttrValue%3A%3AEquals

> Here is an updated patch which replaces the pointer comparison with an
> NS_ABORT_IF_FALSE(false). I'm reluctant to write all those comparison
> functions if I can't test them (and it's not just lengths we need to
> compare, but lists too, and lists of complex stuff--it could end up being
> quite a bit of code).

I added operator== methods to SVGLengthList et. al., so you should only need a small inline function on SVGAnimatedLengthList et. al. that uses that on the base vals of the two objects. That said, if you don't want to write that code please file a followup bug and assign it to me.
Comment on attachment 594927 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them, v1b

Review of attachment 594927 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrValue.cpp
@@ +888,5 @@
> +      if (IsSVGType(thisCont->mType)) {
> +        // Currently this method is never called for nsAttrValue objects that
> +        // point to SVG data types.
> +        // If that changes then we probably want to add methods to the
> +        // corresponding SVG types to compare their base values.

Please also note that we can compare pointers as a shortcut, just so we don't forget to do that.

::: content/base/src/nsAttrValue.h
@@ +128,5 @@
>      ,eAtomArray =      0x11
>      ,eDoubleValue  =   0x12
>      ,eIntMarginValue = 0x13
>      ,eSVGAngle =       0x14
> +    ,eSVGTypesBegin =  eSVGAngle

Please switch eSVGAngle and eSVGTypesBegin so that all the SVG types are bounded by eSVGTypesBegin and eSVGTypesEnd.

@@ +545,5 @@
> +    // All SVG types are just pointers to classes so just setting any of them
> +    // will do. We'll lose type-safety but the signature of the calling
> +    // function should ensure we don't get anything unexpected, and once we
> +    // stick aValue in a union we lose type information anyway.
> +    cont->mSVGAngle = (const nsSVGAngle*)&aValue;

If you're going to make use of this assumption, it seems like you might as well change |const T& aValue| to |const void *aValue| and avoid having the compiler create code for multiple templates.

Please use const_cast and static_cast (or reinterpret_cast if you decide not to do the |const void *aValue| thing) rather than the C-style cast.

@@ +547,5 @@
> +    // function should ensure we don't get anything unexpected, and once we
> +    // stick aValue in a union we lose type information anyway.
> +    cont->mSVGAngle = (const nsSVGAngle*)&aValue;
> +    cont->mType = aType;
> +    if (aSerialized && aSerialized->Length()) {

I think we should just call SetMiscAtomOrString without a check. I *hope* all the SVG types check to see if their baseVal is set when asked for their value as a string, but if any of them were to forget, we'd end up producing non-empty attribute values for default values.

Plus this is really an optimization for attr="" (attribute set to empty string), which doesn't seem worth it to me.
Attachment #594927 - Flags: review?(jwatt) → review+
Comment on attachment 592571 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

I'll wait for a revised 'part 8' addressing the comments in comment 157 (except the "We should be doing object comparison here" comment).

I'll also proceed on the assumption that you will be updating the other patches for which comment 157 comments apply - please let me know if you don't plan to do that.
Attachment #592571 - Flags: review?(jwatt) → review-
Comment on attachment 592572 [details] [diff] [review]
part 9 - Update attribute setting for SVGAnimatedLengthList

Review of attachment 592572 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGLength.cpp
@@ +168,5 @@
>        }
>        return NS_OK;
>      }
>    } else if (mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER ||
>               mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_PX) {

Err, this branch should also be calling DidChangeLengthList - can you fix that while you're touching this code?
Attachment #592572 - Flags: review?(jwatt) → review+
Comment on attachment 592573 [details] [diff] [review]
part 10 - Remove unnecessary serialisation from setting nsSVGNumber2

Review of attachment 592573 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGNumber2.cpp
@@ +152,5 @@
> +nsSVGNumber2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
> +{
> +  if (mIsBaseSet && aValue == mBaseVal) {
> +    return;
> +  }

Hmm, I think we still need a WillModifyNumber method to make sure that restyles happen (see the big doxygen comment requested in comment 157).
Attachment #592573 - Flags: review?(jwatt) → review-
(Reporter)

Updated

6 years ago
Attachment #592574 - Flags: review?(jwatt) → review+
Comment on attachment 592575 [details] [diff] [review]
part 12 - Remove unnecessary serialisation from setting nsSVGInteger

Review of attachment 592575 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGInteger.cpp
@@ +113,5 @@
> +  // attribute value has an associated serialized version (a string value) even
> +  // if the integers match due to the way integers are stored in nsAttrValue.
> +  if (aValue == mBaseVal && mIsBaseSet) {
> +    return;
> +  }

Again, I think we need a WillChangeInteger() call here to make sure restyles happen. r-'ing for now.
Attachment #592575 - Flags: review?(jwatt) → review-
(In reply to Boris Zbarsky (:bz) from comment #160)
> The single atom compare on what's already a slow path is not a big deal for
> the non-SVG case.  So I'd just worry about how the SVG code wants stuff to
> work.

Hmm, I hadn't actually noticed that was an atom compare - I'm not sure we really want to turn SVG list types into atoms given the size of the lists some tools output. Anyway, I guess that's a separate issue to pursue outside this bug.
(Reporter)

Updated

6 years ago
Attachment #592576 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] from comment #166)
> I think we should just call SetMiscAtomOrString without a check. I *hope*
> all the SVG types check to see if their baseVal is set when asked for their
> value as a string, but if any of them were to forget, we'd end up producing
> non-empty attribute values for default values.

Yeah, so it seems nsSVGAngle doesn't have a check to see if its baseVal is actually set.

Probably it would be good to add a check to your test to make sure that attr="" results in the empty string being returned to script from getAttribute("attr").

> Plus this is really an optimization for attr="" (attribute set to empty
> string), which doesn't seem worth it to me.
Comment on attachment 592578 [details] [diff] [review]
part 14 - Remove unnecessary serialisation from setting nsSVGAngle

Review of attachment 592578 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGAngle.cpp
@@ +288,3 @@
>    float valueInUserUnits = mBaseVal * GetDegreesPerUnit(mBaseValUnit);
>    mBaseValUnit = PRUint8(unitType);
> +  SetBaseValue(valueInUserUnits, aSVGElement, false);

Please add a comment noting that you're passing false so that we don't get a double notification.
Attachment #592578 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #592581 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #592582 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #592583 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #592584 - Flags: review?(jwatt) → review+
Comment on attachment 592585 [details] [diff] [review]
part 21 - Remove unnecessary serialisation from setting SVGPathSegList

Review of attachment 592585 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGPathSeg.cpp
@@ +252,2 @@
>        InternalItem()[1+index] = float(a##propName);                           \
>        NS_ABORT_IF_FALSE(IsInList(), "DidChangePathSegList() is wrong");       \

You can move this abort up to before the WillChangePathSegList() call, and change it to "Will/DidChangePathSegList() is wrong".
Attachment #592585 - Flags: review?(jwatt) → review+
Comment on attachment 592586 [details] [diff] [review]
part 22 - Remove unnecessary serialisation from setting SVGTransformList

Review of attachment 592586 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGTransform.cpp
@@ +181,5 @@
>  NS_IMETHODIMP
>  DOMSVGTransform::SetTranslate(float tx, float ty)
>  {
> +  if (mIsAnimValItem)
> +    return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR;

Not really keen on all this curly bracket removing noise, especially in such a big patch series (and because I prefer them), but I guess you've done it now.

::: content/svg/content/src/DOMSVGTransform.h
@@ +205,5 @@
>    }
>    SVGTransform& Transform() {
>      return HasOwner() ? InternalItem() : *mTransform;
>    }
> +  nsAttrValue NotifyElementWillChange();

If this was inline we'd benefit from better RVO, but not a big deal.
Attachment #592586 - Flags: review?(jwatt) → review+
Created attachment 595008 [details] [diff] [review]
placeholder patch for addressing comment 155
Attachment #595008 - Flags: review-
The landing on inbound caused a 10-15% regression on dromaeo-DOM.  Why?  Nothing in there looked obviously bad to me, but clearly something bad is happening.

Brian, can you please back out of inbound for now, then figure out which of those changes is regressing dromaeo-DOM (and ideally which subtest it's regressing and by how much)?
I backed it out of inbound, for the perf regressions.

Updated

6 years ago
Whiteboard: [inbound, parts 1 to 5 only, don't close yet please]
> I'm not sure we really want to turn SVG list types into atoms

We only do that when we have mutation event listeners.  I wouldn't worry about it overmuch for now.

Back to the perf regression.... is it possible that it's due to the cost of having to make a copy of the string every time before doing the "has the value changed?" compare?  If it is (again, testing which patch caused the problem and which tests are affected plus some profiling would likely tell us), we may want to pass both an nsAttrValue and a string to the function that checks for changes, and use whichever one has data.....  Alternately, we could have a way to create an nsAttrValue out of a dependent string somehow, maybe.
I took a look at the perf regression to see if I can help speed things along a bit.

It seems to be the dom-attr tests that have regressed: http://dromaeo.com/?dom-attr . Specifically, it seems like 'part 1' regresses the 'setAttribute' and 'element.property = value' tests (they test setting the 'id' attribute to the same value over and over again), and 'part 2' regresses the 'element.property' test (which tests reading the 'id' attribute (same value) over and over again). The subsequent three patches seem to have little effect. You can see the script these tests run at the top of:

view-source:http://dromaeo.com/tests/dom-attr.html

Just from looking at the code in a debugger, there are two things that seem to add overhead in part 1, to me. First in SetAttr there's the line 'nsAttrValue value(aValue)', which causes us to allocate memory for and copy a new string. Then there's the fact that in MaybeCheckSameAttrVal |aValue.EqualsAsStrings(*info.mValue)| is more expensive than the old |info.mValue->Equals(aValue, eCaseMatters)|. The reason that EqualsAsStrings is more expensive is because the nsAttrValue for the 'id' attribute's existing value is of type eAtom, whereas the new nsAttrValue is of type eString. This causes EqualsAsStrings to convert the eAtom value to a string (allocating and copying) for comparison, whereas the old code just used to call Equals() on the atom.

Anyway, I'll get some profiles shortly.
Created attachment 595215 [details]
Profile before 'part 1' patch - formatting screwed up
Created attachment 595216 [details]
Profile after 'part 1' patch - formatting screwed up
(Reporter)

Updated

6 years ago
Attachment #595215 - Attachment description: Profile before 'part 1' patch → Profile before 'part 1' patch - formatting screwed up
Attachment #595215 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #595216 - Attachment description: Profile after 'part 1' patch → Profile after 'part 1' patch - formatting screwed up
Attachment #595216 - Attachment is obsolete: true
Created attachment 595255 [details]
Profile before 'part 1' patch
(Reporter)

Updated

6 years ago
Attachment #595255 - Attachment description: Profile after 'part 1' patch → Profile before 'part 1' patch
Created attachment 595256 [details]
Profile after 'part 1' patch
So it seems like before 'part 1', MaybeCheckSameAttrVal completely dominates nsGenericElement::SetAttr, then after 'part 1' the nsAttrValue ctor and dtor calls dominate, *even though MaybeCheckSameAttrVal got a bit more expensive*!
> First in SetAttr there's the line 'nsAttrValue value(aValue)', which causes us to
> allocate memory for and copy a new string.

Right, see comment 179.  Looking at your profile data, that's a _huge_ cost.  Just the constructor and destructor for that nsAttrValue in the attached profile are 1.5x what the total time was for the old code!

> This causes EqualsAsStrings to convert the eAtom value to a string

Hmm.  This is definitely wrong, if it's happening.  It looks like lhs and rhs are backwards in EqualsAsStrings?  We want to serialize the one that's already of string type, if any.  That said, this doesn't seem to be a huge hit.
Isn't nsAttrValue::SetTo(const nsAString& aValue) already attempting to share the source string's buffer?
Yes, but the source string in this case is coming from JS, so it's a dependent string pointing to the JSString data and hence has no buffer to share.

We really need to fix that.  :(
Then seems like passing the string alongside the nsAttrValue to MaybeCheckSameAttrVal is the easiest way to fix this.
(Assignee)

Comment 190

6 years ago
Created attachment 595294 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings

This patch fixes the perf regression with regards to setAttribute along the lines of comment 189. I also tweaked EqualsAsStrings.

(In reply to Boris Zbarsky (:bz) from comment #186)
> It looks like lhs and
> rhs are backwards in EqualsAsStrings?  We want to serialize the one that's
> already of string type, if any.

They're the right way around. nsAttrValue::Equals(const nsAString&, nsCaseTreatment) will avoid serialising its own value if it's a string or atom type and use a dependent string instead.

That said, EqualsAsStrings could be optimised further. After establishing the lhs and rhs, it could check if the rhs is of string/atom type and then use GetStringValue()/GetAtomValue() and pass those to Equals rather than using ToString. It's not relevant to this particular test anymore now that I've followed Rob's suggestion and passing either the string or nsAttrValue along but might still be worth doing.

Not putting this up for review yet because:
* There is still a significant perf regression in createTextNode. I can't figure out why yet.
* I should add the documentation requested in comment 142.
* I need rebase the other patches on top and check it all still works.
* I need to run it through Try and check the perf regression is fixed on other platforms too (currently tested only on Win)
(Assignee)

Updated

6 years ago
Attachment #594643 - Attachment is obsolete: true
Attachment #594643 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #592951 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #594644 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #592564 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #592568 - Flags: checkin+
> They're the right way around.

Ah, with the atom check added, agreed.

> There is still a significant perf regression in createTextNode.

Erm.  That's pretty odd.  From which patch, just part 1?
(Assignee)

Comment 192

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #191)
> > There is still a significant perf regression in createTextNode.
> 
> Erm.  That's pretty odd.  From which patch, just part 1?

Yes, just part 1. It is odd and I think it's just noise. I've run it a few more times and the results vary alot (the error range dromaeo gives me is about 40% for that result). Looking at it in a profiler, most of the work is in XPC and I can't see any other code that this patch touches.

I'm going to ignore it for now and see what results Try gives me later.
(Assignee)

Comment 193

6 years ago
(In reply to Brian Birtles (:birtles) from comment #190)
> * I need rebase the other patches on top and check it all still works.

I guess I didn't think through these changes at all. The first few patches in this series are all about replacing nsAString with nsAttrValue.

The problem, of course, is that this is currently expensive for strings from JS (comment 188).

So possible solutions:
a) Allow sharing string buffers with JSString data--this sounds like it would take a long time and bz assures me we shouldn't wait for that to happen.

b) Make a stack class that wraps a pointer to a string or an nsAttrValue and pass that to BeforeSetAttr etc. It could have convenience methods for getting a string out of it etc.

c) Make a stack subclass of nsAttrValue that can be initialised with a dependent string. This is bz's suggestion. I'm not quite clear on all the details but I guess we could do it without making method calls virtual by putting all the functionality into nsAttrValue itself.

After talking with bz we think (c) is worth a try. Jonas, what do you think?
So what I was thinking of is that we have a stack-only subclass of nsAttrValue that can be initialized with a |const nsAString*|.  It would use protected methods on the nsAttrValue to initialize it with that pointer.  We'd ned a new attr value type id for this "dependent string" case.  And then we'd need to change various logic in nsAttrValue to handle this new value type....  I think it should work ok.
(Assignee)

Comment 195

6 years ago
Created attachment 595336 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings;

Removed the passing of both strings and nsAttrValue to MaybeCheckSameAttrVal since it's not going to work for us after all. Added documentation to nsAttrValue.h.
(Assignee)

Updated

6 years ago
Attachment #595294 - Attachment is obsolete: true
(Assignee)

Comment 196

6 years ago
Created attachment 595337 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

(In reply to Jonathan Watt [:jwatt] from comment #157)
> Comment on attachment 592571 [details] [diff] [review]
> part 8 - Remove unnecessary serialisation from setting nsSVGLength2
> Again, a Doxygen style comment would be good:
> 
> /**
>  * Helper methods for the type-specific DidChangeXXX methods.
>  *
>  * aEmptyOrOldValue must be the object returned from the corresponding
>  * WillChangeXXX call. If we have mutation event listeners things will go
>  * wrong if it isn't.
>  */

I expanded this comment a little since "things will go wrong" sounds a little mysterious.
 
> @@ +1322,5 @@
> > +
> > +void
> > +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,
> > +                             nsAttrValue& aNewValue)
> > +{
> 
> It would be good if we could have an NS_ABORT_IF_FALSE below checking that
> if we have mutation listeners, aEmptyOrOldValue is not uninitialized. I'm
> not really sure how you can do that currently though, since the best
> nsAttrValue seems to offer is IsEmptyString(), which is not what we want.
> Maybe we can add a bogus "not initialized" type to nsAttrValue purely for
> this purpose? Boris/Jonas?

It would be nice to be able to test we're doing the right thing here with an assertion. However, I wonder if what we have is enough for now. Firstly, nothing horrible will happen if aOldValue is an empty string since SetAttrAndNotify already handle empty strings. The problem is just correctness and for now the tests I've added should cover this (i.e. we're checking the oldValue member of mutation events for a pretty good range of mutations). Maybe it's sufficient to rely on the tests for now?

> @@ +346,4 @@
> >    float valueInUserUnits = 
> >      mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);
> >    mSpecifiedUnitType = PRUint8(unitType);
> > +  SetBaseValue(valueInUserUnits, aSVGElement, false);
> 
> Hmm, under SetBaseValue() we call SetBaseValueInSpecifiedUnits(), so we'll
> get two Will/DidModifyLength calls for each ConvertToSpecifiedUnits() call...

No it's fine since aDoSetAttr is false. We have tests that would catch this if we were sending 
duplicate notifications.
Attachment #592571 - Attachment is obsolete: true
Attachment #595337 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Blocks: 725227
(In reply to Boris Zbarsky (:bz) from comment #186)
> Right, see comment 179.

Sorry, I didn't mean to ignore your observations, I was just trying to provide more info. :)
Component: SVG → Style System (CSS)
I did some profiling of the 'part 2' regression this morning. I was seeing a fairly significant drop (50 -> 40) yesterday, but I'm not seeing that today (I'm still using the same patches that were checked in on m-i). Something that I should have been keeping constant must have varied yesterday and given me bad results.

The measurements I got today are as follows (number proceeding each group of three lines indicates the patch applied for that measurement - "0" means no patches):

0:
33.85  44.28  32.35  20.03  12.30  28.06  http://dromaeo.com/?id=162807
33.30  42.88  32.94  20.29  12.18  27.92  http://dromaeo.com/?id=162809
33.33  41.64  31.62  20.08  12.33  27.71  http://dromaeo.com/?id=162810
1:
31.30  42.98  15.99  11.50  12.15  28.28  http://dromaeo.com/?id=162803
31.84  38.61  16.09  11.45  11.76  28.53  http://dromaeo.com/?id=162804
31.88  43.01  16.13  11.38  11.94  28.38  http://dromaeo.com/?id=162805
2:
32.44  46.34  16.11  11.93  12.37  28.90  http://dromaeo.com/?id=162800
31.45  44.22  15.76  11.83  12.56  28.65  http://dromaeo.com/?id=162801
31.87  41.78  15.94  11.91  12.54  29.09  http://dromaeo.com/?id=162802
5:
31.27  43.77  16.21  12.13  12.07  28.02  http://dromaeo.com/?id=162814
31.23  38.84  16.23  12.10  12.24  27.63  http://dromaeo.com/?id=162815
30.79  42.26  16.53  12.06  12.19  28.14  http://dromaeo.com/?id=162816

So it seems like only part 1 regresses the dom-attr tests, and only the 'setAttribute' and 'element.property = value' subtests.
Assignee: birtles → nobody
Component: Style System (CSS) → SVG
(What is going on with these random field changes - seriously, I never touched anything at the top of the bug!)
Assignee: nobody → birtles
(Assignee)

Comment 200

6 years ago
Created attachment 595630 [details] [diff] [review]
part 6 - Add test framework for modification events
(Assignee)

Updated

6 years ago
Attachment #592569 - Attachment is obsolete: true
Attachment #592569 - Flags: review?(jwatt)
(Assignee)

Comment 201

6 years ago
Created attachment 595631 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595630 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #595631 - Attachment description: part 7 - Remove unnecessary serialisation from setting SVGEnum; → part 7 - Remove unnecessary serialisation from setting SVGEnum; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #594645 - Attachment is obsolete: true
(Assignee)

Comment 202

6 years ago
Created attachment 595632 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2
(Assignee)

Updated

6 years ago
Attachment #595337 - Attachment is obsolete: true
Attachment #595337 - Flags: review?(jwatt)
(Assignee)

Comment 203

6 years ago
Created attachment 595633 [details] [diff] [review]
part 9 - Update attribute setting for SVGAnimatedLengthList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592572 - Attachment is obsolete: true
(Assignee)

Comment 204

6 years ago
Created attachment 595634 [details] [diff] [review]
part 10 - Remove unnecessary serialisation from setting nsSVGNumber2
(Assignee)

Updated

6 years ago
Attachment #592573 - Attachment is obsolete: true
(Assignee)

Comment 205

6 years ago
Created attachment 595635 [details] [diff] [review]
part 11 - Remove unnecessary serialisation from setting nsSVGNumberPair; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595632 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #592574 - Attachment is obsolete: true
(Assignee)

Comment 206

6 years ago
Created attachment 595636 [details] [diff] [review]
part 12 - Remove unnecessary serialisation from setting nsSVGInteger
(Assignee)

Updated

6 years ago
Attachment #595634 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #592575 - Attachment is obsolete: true
(Assignee)

Comment 207

6 years ago
Created attachment 595637 [details] [diff] [review]
part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592576 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #595636 - Flags: review?(jwatt)
(Assignee)

Comment 208

6 years ago
Created attachment 595638 [details] [diff] [review]
part 14 - Remove unnecessary serialisation from setting nsSVGAngle; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592578 - Attachment is obsolete: true
(Assignee)

Comment 209

6 years ago
Created attachment 595639 [details] [diff] [review]
part 15 - Remove unnecessary serialisation from setting nsSVGBoolean
(Assignee)

Updated

6 years ago
Attachment #592579 - Attachment is obsolete: true
Attachment #592579 - Flags: review?(jwatt)
(Assignee)

Comment 210

6 years ago
Created attachment 595640 [details] [diff] [review]
part 16 - Add mutation event tests for strings and classes and tidy up use of DidChangeString
(Assignee)

Updated

6 years ago
Attachment #592580 - Attachment is obsolete: true
Attachment #592580 - Flags: review?(jwatt)
(Assignee)

Comment 211

6 years ago
Created attachment 595641 [details] [diff] [review]
part 17 - Remove unnecessary serialisation from setting nsSVGPreserveAspectRatio; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592581 - Attachment is obsolete: true
(Assignee)

Comment 212

6 years ago
Created attachment 595642 [details] [diff] [review]
part 18 - Remove unnecessary serialisation from setting nsSVGViewBox; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592582 - Attachment is obsolete: true
(Assignee)

Comment 213

6 years ago
Created attachment 595643 [details] [diff] [review]
part 19 - Remove unnecessary serialisation from setting nsSVGNumberList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592583 - Attachment is obsolete: true
(Assignee)

Comment 214

6 years ago
Created attachment 595644 [details] [diff] [review]
part 20 - Remove unnecessary serialisation from setting SVGPointList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592584 - Attachment is obsolete: true
(Assignee)

Comment 215

6 years ago
Created attachment 595645 [details] [diff] [review]
part 21 - Remove unnecessary serialisation from setting SVGPathSegList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592585 - Attachment is obsolete: true
(Assignee)

Comment 216

6 years ago
Created attachment 595646 [details] [diff] [review]
part 22 - Remove unnecessary serialisation from setting SVGTransformList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #592586 - Attachment is obsolete: true
(Assignee)

Comment 217

6 years ago
Created attachment 595647 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch; r=bz
(Assignee)

Updated

6 years ago
Attachment #592587 - Attachment is obsolete: true
Attachment #592587 - Flags: review?(jwatt)
(Assignee)

Comment 218

6 years ago
Created attachment 595648 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #594927 - Attachment is obsolete: true
(Assignee)

Comment 219

6 years ago
Created attachment 595649 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList
(Assignee)

Updated

6 years ago
Attachment #594650 - Attachment is obsolete: true
Attachment #594650 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #595639 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #595640 - Flags: review?(jwatt)
(Assignee)

Updated

6 years ago
Attachment #595649 - Flags: review?(jwatt)
(Assignee)

Comment 220

6 years ago
Comment on attachment 595647 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch; r=bz

I'm asking for review on this even though Boris has already given it r+ because I think Boris' r+ was just to say the changes to nsGenericElement are ok but he'll leave it to us to decide what's best on the SVG side. We talked about this in IRC.

Basically, SetParsedAttr compares old and new values and filters out redundant changes so notifications aren't sent. With the new Will/DoChange scheme we lose that and have to filter those changes out ourselves. That's a fair bit of code and it's fragile. That's why in this patch I got rid of all that code and put an extra atom comparison in between the old and new values just before we send events.

I think we agree that the performance cost of this for nsGenericElement is fine. The question is just whether it's better to catch these redundant changes earlier in SVG land. It probably is so I'm happy to just skip this patch altogether.
Attachment #595647 - Flags: review?(jwatt)
(Assignee)

Comment 221

6 years ago
With this latest blitz I think I have addressed all review feedback up to date with the exception of writing the extra nsAttrValue subclass to fix the perf regression (see comment 194). I'll hopefully get to that tomorrow.

(In reply to Jonathan Watt [:jwatt] from comment #168)
> Comment on attachment 592572 [details] [diff] [review]
> part 9 - Update attribute setting for SVGAnimatedLengthList
> 
> Review of attachment 592572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/svg/content/src/DOMSVGLength.cpp
> @@ +168,5 @@
> >        }
> >        return NS_OK;
> >      }
> >    } else if (mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER ||
> >               mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_PX) {
> 
> Err, this branch should also be calling DidChangeLengthList - can you fix
> that while you're touching this code?

This is the branch where HasOwner() is false, which means we have no element to call DidChangeLengthList on. I think this is correct as is.

> @@ +547,5 @@
> > +    // function should ensure we don't get anything unexpected, and once we
> > +    // stick aValue in a union we lose type information anyway.
> > +    cont->mSVGAngle = (const nsSVGAngle*)&aValue;
> > +    cont->mType = aType;
> > +    if (aSerialized && aSerialized->Length()) {
> 
> I think we should just call SetMiscAtomOrString without a check. I *hope*
> all the SVG types check to see if their baseVal is set when asked for their
> value as a string, but if any of them were to forget, we'd end up producing
> non-empty attribute values for default values.
> 
> Plus this is really an optimization for attr="" (attribute set to empty
> string), which doesn't seem worth it to me.

The reason for this check is that nsAttrValue::SetMiscAtomOrString asserts that non-null strings are also not empty (with exceptions for CSSStyleRule and Enum types). For most SVG types, e.g. angles, if we get an empty string it won't parse and we'll just set the attribute using the string interface, not the SVG-type interface.

However, for some list types, empty attributes are still set using the SVG-type interface so we can arrive at nsAttrValue::SetTo(const mozilla::SVGPathData&, const nsAString*) with an empty string.

Some possibilities:
a) Catch this case for those types where empty strings are allowed and pass on a null pointer to SetSVGType
b) Update the assertion in SetMiscAtomOrString to allow these types (or even all SVG types)
c) Just detect empty strings in SetSVGType (i.e. for all SVG types) and don't store them

We currently do (c). I looked at doing (b) but the current exceptions to the assertion are for cases where storing an empty string is useful (as opposed to storing them because we don't feel like detecting them). For now I've done (a).

(In reply to Jonathan Watt [:jwatt] from comment #169)
> Comment on attachment 592573 [details] [diff] [review]
> part 10 - Remove unnecessary serialisation from setting nsSVGNumber2
> 
> Review of attachment 592573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/svg/content/src/nsSVGNumber2.cpp
> @@ +152,5 @@
> > +nsSVGNumber2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
> > +{
> > +  if (mIsBaseSet && aValue == mBaseVal) {
> > +    return;
> > +  }
> 
> Hmm, I think we still need a WillModifyNumber method to make sure that
> restyles happen (see the big doxygen comment requested in comment 157).

We basically have two categories:
a) SVG types where we can store the data in the nsAttrValue
b) SVG types where we just put a pointer in the nsAttrValue to the SVG object

(a) includes bools, enums, integers etc. For these types we don't need the Will/DidChange pattern. We can just call SetParsedAttr and it will fetch the old independent value from mAttrsAndChildren if needed. It also does everything that our WillChangeXXX methods do (that's what they're based on).

It basically works the same as things do without this patch except that instead of storing the value in the nsAttrValue as a string it's stored as an integer.

I've updated the comment to cover this. The same goes for comment 170 (part 12). 

(In reply to Brian Birtles (:birtles) from comment #196)
> > Hmm, under SetBaseValue() we call SetBaseValueInSpecifiedUnits(), so we'll
> > get two Will/DidModifyLength calls for each ConvertToSpecifiedUnits() call...
> 
> No it's fine since aDoSetAttr is false. We have tests that would catch this
> if we were sending 
> duplicate notifications.

Added a comment for this now.
Comment on attachment 595632 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

Review of attachment 595632 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrAndChildArray.h
@@ +135,5 @@
>             !AttrSlotIsTaken(ATTRCHILD_ARRAY_MAX_ATTR_COUNT - 1);
>    }
>  
>    PRInt64 SizeOf() const;
> +  bool HasMappedAttrs() const { return MappedAttrCount(); }

Format the curly brackets the same as the other methods in this file, please.

::: content/svg/content/src/nsSVGElement.cpp
@@ +261,5 @@
>  nsresult
>  nsSVGElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>                             const nsAttrValue* aValue, bool aNotify)
>  {
> +  // We don't currently use mapped attributes within SVG. If this changes, we

We do have "mapped attributes", we just don't use nsMappedAttributes. Please s/mapped attributes/nsMappedAttributes/ here.

@@ +264,5 @@
>  {
> +  // We don't currently use mapped attributes within SVG. If this changes, we
> +  // need to be very careful because some nsAttrValues used by SVG point to
> +  // member data of SVG elements and if an nsAttrValue outlives the SVG element
> +  // whose data it points to, the pointer will dangle. See bug 724680.

After "whose data it points to" please add "(by virtue of being stored in mAttrsAndChildren->mMappedAttributes, meaning it's shared between elements)".

@@ +266,5 @@
> +  // need to be very careful because some nsAttrValues used by SVG point to
> +  // member data of SVG elements and if an nsAttrValue outlives the SVG element
> +  // whose data it points to, the pointer will dangle. See bug 724680.
> +  NS_ABORT_IF_FALSE(!mAttrsAndChildren.HasMappedAttrs(),
> +    "Unexpected used of mapped attributes used within SVG");

s/mapped attributes/nsMappedAttributes/ here too.

@@ +1275,5 @@
> + * The nsAttrValue returned by this method depends on whether there are
> + * mutation event listeners listening for changes to this element's attributes.
> + * If not, then the object returned is empty. If there are, then the
> + * nsAttrValue returned contains a serialized copy of the attribute's value
> + * prior to the change, and this object should be passed to the corresponding

s/should/_MUST_/

@@ +1276,5 @@
> + * mutation event listeners listening for changes to this element's attributes.
> + * If not, then the object returned is empty. If there are, then the
> + * nsAttrValue returned contains a serialized copy of the attribute's value
> + * prior to the change, and this object should be passed to the corresponding
> + * DidChangeXXX method call. This is necessary so that the 'prevValue' property

s/call/call (assuming a WillModifyXXX call is required for the SVG type - see comment below).

::: content/svg/content/src/nsSVGElement.h
@@ +259,5 @@
> +  // BeforeSetAttr since it would involve allocating extra SVG value types.
> +  // See the comment in nsSVGElement::WillChangeValue.
> +  virtual nsresult BeforeSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
> +                                 const nsAttrValue* aValue,
> +                                 bool aNotify) MOZ_FINAL { return NS_OK; }

Awesome, thank you. :)
Attachment #595632 - Flags: review?(jwatt) → review+
Comment on attachment 595648 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them; r=jwatt

Review of attachment 595648 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrValue.h
@@ +139,5 @@
>      ,eSVGPointList  =  0x21
>      ,eSVGPreserveAspectRatio = 0x22
>      ,eSVGTransformList = 0x23
>      ,eSVGViewBox =     0x24
> +    ,eSVGTypesEnd =    eSVGViewBox

Hmm, actually we should probably set eSVGTypesEnd to 0x34 or something, to give ourselves a buffer for new types to be added.

Would that be okay with your Boris?
(Reporter)

Updated

6 years ago
Attachment #595008 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #595634 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #595636 - Flags: review?(jwatt) → review+
Comment on attachment 595648 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them; r=jwatt

Temporary r- due to the:

+  // While an empty string will parse as a number list, there's no need to store
+  // it (and SetMiscAtomOrString will assert if we try)
+  if (aSerialized && !aSerialized->Length()) {
+    aSerialized = nsnull;
+  }

code which I'm not sure about. I think we still have the problem described in comment 172 for some types.

We need to make sure we have tests for all types that check that:

setAttribute("");
ok(getAttribute() === "");

and:

removeAttribute();
ok(getAttribute() === null);

and:

baseVal.clear();
ok(hasAttribute());
ok(getAttribute() === "");

Note the use of ===, and ok(), not is(). This is because is() doesn't to a strict equals comparison.

In test_SVGxxxList.xhtml, in run_baseVal_API_tests(), you can add those checks right before the "// Test .initialize():" comment.

For the non-list types I'm not sure offhand where to add those checks. Let me know if you need help tracking down appropriate spots.
Attachment #595648 - Flags: review-
(In reply to Brian Birtles (:birtles) from comment #220)
> I think we agree that the performance cost of this for nsGenericElement is
> fine. The question is just whether it's better to catch these redundant
> changes earlier in SVG land. It probably is so I'm happy to just skip this
> patch altogether.

Can we have a separate bug for it?
(Reporter)

Updated

6 years ago
Attachment #595639 - Flags: review?(jwatt) → review+
(Reporter)

Updated

6 years ago
Attachment #595640 - Flags: review?(jwatt) → review+
Comment on attachment 595630 [details] [diff] [review]
part 6 - Add test framework for modification events

Review of attachment 595630 [details] [diff] [review]:
-----------------------------------------------------------------

Can you replace the is() calls with ok() calls and the === or !== operators wherever you're comparing to the empty string?

::: content/svg/content/test/MutationEventChecker.js
@@ +36,5 @@
> +function MutationEventChecker()
> +{
> +  this.expectedEvents = [];
> +
> +  this.checkAttr = function(element, attr)

This sounds like the imperative, and at the callers it looks like the checks are done under the call. How about calling it watchAttr?

@@ +152,5 @@
> +         'Unexpected old value for modification to ' + this.attr +
> +         ' attribute');
> +      isnot(e.newValue, this.oldValue,
> +         'Unexpected new value for modification to ' + this.attr +
> +         ' attribute');

Add a this.element.hasAttribute() check here too.

@@ +160,5 @@
> +         'Unexpected old value for removal of ' + this.attr +
> +         ' attribute');
> +      is(e.newValue, "",
> +         'Unexpected new value for removal of ' + this.attr +
> +         ' attribute');

Add a this.element.hasAttribute() check here.

@@ +168,5 @@
> +         'Unexpected old value for addition of ' + this.attr +
> +         ' attribute');
> +      isnot(e.newValue, "",
> +         'Unexpected new value for addition of ' + this.attr +
> +         ' attribute');

Add a check for this.element.getAttribute here too.

@@ +170,5 @@
> +      isnot(e.newValue, "",
> +         'Unexpected new value for addition of ' + this.attr +
> +         ' attribute');
> +      this.oldValue = e.newValue;
> +    }

Put a trailing 'else' with an ok(false, "unexpected mutation event type") here.

Seems like you should also be able to put the this.oldValue setting lines after that, or is e.newValue not |undefined| in the removal case for some reason?
> Would that be okay with your Boris?

Just so we don't have to update the "end" value when we add a new type?  I'm pretty happy just updating it if we need to.
No, to avoid having to modify any other types that other people add in the interim (presumably they'll start at 0x25 - my suggestion was to head that off and make them start at a higher number).
Ah, ok.  Sure.
(Assignee)

Comment 230

6 years ago
Created attachment 595951 [details] [diff] [review]
part 6 - Add test framework for modification events; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595630 - Attachment is obsolete: true
Attachment #595630 - Flags: review?(jwatt)
(Assignee)

Comment 231

6 years ago
Created attachment 595952 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595632 - Attachment is obsolete: true
(Assignee)

Comment 232

6 years ago
Created attachment 595953 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595648 - Attachment is obsolete: true
(Assignee)

Comment 233

6 years ago
(In reply to Jonathan Watt [:jwatt] from comment #222)
> Comment on attachment 595632 [details] [diff] [review]
> part 8 - Remove unnecessary serialisation from setting nsSVGLength2
...
> @@ +1275,5 @@
> > + * The nsAttrValue returned by this method depends on whether there are
> > + * mutation event listeners listening for changes to this element's attributes.
> > + * If not, then the object returned is empty. If there are, then the
> > + * nsAttrValue returned contains a serialized copy of the attribute's value
> > + * prior to the change, and this object should be passed to the corresponding
> 
> s/should/_MUST_/

Yeah I cheekily changed that because it didn't make sense to me. It seems conceivable that someone could make an nsAttrValue with roughly the same information in it and pass that along instead (perhaps because there was an side effect of WillChangeXXX they wanted to avoid so they did a similar routine themselves). Assuming they've read that sentence about the fact that the value should be fixed (not pointing somewhere else) then I can't see a reason why you couldn't use another object. Or maybe I'm missing something?

(In reply to Jonathan Watt [:jwatt] from comment #224)
> Comment on attachment 595648 [details] [diff] [review]
> part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to
> export them; r=jwatt
> In test_SVGxxxList.xhtml, in run_baseVal_API_tests(), you can add those
> checks right before the "// Test .initialize():" comment.
> 
> For the non-list types I'm not sure offhand where to add those checks. Let
> me know if you need help tracking down appropriate spots.

I already added all the empty string tests in the previous round of patches. (The tests are added on a per-type basis, mostly to test_dataTypes.html)

I've gone through now and replaced is() with ok() and added the extra check for removeAttribute. (clear() only applies to lists and I've updated the lists test accordingly there.)

I won't post the updated patches just yet to avoid bug spam since they'll all have to be updated again after the new nsAttrValue subclass is added.

> Note the use of ===, and ok(), not is(). This is because is() doesn't to a
> strict equals comparison.

Oh man, in most unit test frameworks is() does the strict check. That's the main point of using it (the other being it gives you better error messages). Why does MochiTest have to be different? :) Fixed anyway.

> part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to
> export them; r=jwatt
> 
> Temporary r- due to the:
> 
> +  // While an empty string will parse as a number list, there's no need to
> store
> +  // it (and SetMiscAtomOrString will assert if we try)
> +  if (aSerialized && !aSerialized->Length()) {
> +    aSerialized = nsnull;
> +  }
> 
> code which I'm not sure about. I think we still have the problem described
> in comment 172 for some types.

Even with all the new tests everything passes so I think this is fine based on the middle of comment 221. That is, we don't use nsSVGAngle to represent empty strings because it doesn't parse. If it did, we'd at least get an assertion in nsSVGAttrValue.h when we try to store it. And now a failing test too.

(In reply to Jonathan Watt [:jwatt] from comment #225)
> (In reply to Brian Birtles (:birtles) from comment #220)
> > I think we agree that the performance cost of this for nsGenericElement is
> > fine. The question is just whether it's better to catch these redundant
> > changes earlier in SVG land. It probably is so I'm happy to just skip this
> > patch altogether.
> 
> Can we have a separate bug for it?

For which? For removing the patch? Why not just drop it now? What I'm suggesting is there are three possibilities:

a) Filter out redundant changes in SVG land = don't land part 23, just obsolete it
b) Filter out redundant changes by comparing atoms before dispatching the event = land part 23
c) Filter out redundant changes in SVG land and, as a backup, compare atoms before dispatching the event = land just the first hunk of part 23 (the first 34 lines)

I lean towards (a) but am ok with (b).

I'm not a big fan of (c) because if we think (a) is more efficient, then the fallback will stop us from detecting cases where we're suboptimal.
(Assignee)

Updated

6 years ago
Attachment #595953 - Flags: review?(jwatt)
(In reply to Brian Birtles (:birtles) from comment #233)
> Assuming they've read that sentence about the fact that
> the value should be fixed (not pointing somewhere else) then I can't see a
> reason why you couldn't use another object.

Okay.

> I already added all the empty string tests in the previous round of patches.
> (The tests are added on a per-type basis, mostly to test_dataTypes.html)
> 
> I've gone through now and replaced is() with ok() and added the extra check
> for removeAttribute. (clear() only applies to lists and I've updated the
> lists test accordingly there.)

Okay, thanks!

> I won't post the updated patches just yet to avoid bug spam since they'll
> all have to be updated again after the new nsAttrValue subclass is added.

If that's going to take more than a day, can you provide me with a quick roll-up patch of what you have just now?

> Oh man, in most unit test frameworks is() does the strict check. That's the
> main point of using it (the other being it gives you better error messages).
> Why does MochiTest have to be different? :) Fixed anyway.

Yeah, it's annoying.

> Even with all the new tests everything passes so I think this is fine based
> on the middle of comment 221. That is, we don't use nsSVGAngle to represent
> empty strings because it doesn't parse. If it did, we'd at least get an
> assertion in nsSVGAttrValue.h when we try to store it. And now a failing
> test too.

Okay. I don't have time to test myself, so I'll trust you on this one. ;)

> > Can we have a separate bug for it?
> 
> For which? For removing the patch? Why not just drop it now?

No, a separate bug for considering *taking* the patch as a follow-up. I want to consider it more, but don't want to block this bug on that.
(Assignee)

Comment 235

6 years ago
Created attachment 596565 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz
(Assignee)

Updated

6 years ago
Attachment #595336 - Attachment is obsolete: true
(Assignee)

Comment 236

6 years ago
Created attachment 596566 [details] [diff] [review]
part 2 - Make BeforeSetAttr take an nsAttrValue; r=bz
(Assignee)

Updated

6 years ago
Attachment #592564 - Attachment is obsolete: true
(Assignee)

Comment 237

6 years ago
Created attachment 596567 [details] [diff] [review]
part 3 - Make AfterSetAttr take an nsAttrValue; r=bz
(Assignee)

Updated

6 years ago
Attachment #594644 - Attachment is obsolete: true
(Assignee)

Comment 238

6 years ago
Created attachment 596571 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz
(Assignee)

Updated

6 years ago
Attachment #596565 - Attachment is obsolete: true
(Assignee)

Comment 239

6 years ago
Comment on attachment 596571 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz

Hi Boris, would you mind having another look at this?

I started down the path of making a subclass for nsAttrValue but it was a bit messy. (For example, with the subclass presumably GetStringValue() should return the dependent string--otherwise call sites are really complicated, but Type() should return something other than eString since it's used internally.) Then I realised we only need to have both the string and nsAttrValue around for nsGenericElement::MaybeCheckSameValue and nsGenericElement::BeforeSetAttr since after that point we have an nsAttrValue for both code paths.

So I've gone ahead with the simpler approach of just making a wrapper, nsAttrStringOrValue, which contains the two and makes getting to the string value easy so call sites are simple.
Attachment #596571 - Flags: review?(bzbarsky)
Comment on attachment 596571 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz

r=me
Attachment #596571 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Blocks: 726523
(Assignee)

Comment 241

6 years ago
Comment on attachment 595647 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch; r=bz

Moved to bug 726523
Attachment #595647 - Attachment is obsolete: true
Attachment #595647 - Flags: review?(jwatt)
(Assignee)

Comment 242

6 years ago
Created attachment 596587 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595631 - Attachment is obsolete: true
(Assignee)

Comment 243

6 years ago
Created attachment 596588 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595952 - Attachment is obsolete: true
(Assignee)

Comment 244

6 years ago
Created attachment 596589 [details] [diff] [review]
part 9 - Update attribute setting for SVGAnimatedLengthList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595633 - Attachment is obsolete: true
(Assignee)

Comment 245

6 years ago
Created attachment 596590 [details] [diff] [review]
part 10 - Remove unnecessary serialisation from setting nsSVGNumber2; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595634 - Attachment is obsolete: true
(Assignee)

Comment 246

6 years ago
Created attachment 596591 [details] [diff] [review]
part 11 - Remove unnecessary serialisation from setting nsSVGNumberPair; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595635 - Attachment is obsolete: true
(Assignee)

Comment 247

6 years ago
Created attachment 596593 [details] [diff] [review]
part 12 - Remove unnecessary serialisation from setting nsSVGInteger; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595636 - Attachment is obsolete: true
(Assignee)

Comment 248

6 years ago
Created attachment 596594 [details] [diff] [review]
part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595637 - Attachment is obsolete: true
(Assignee)

Comment 249

6 years ago
Created attachment 596595 [details] [diff] [review]
part 14 - Remove unnecessary serialisation from setting nsSVGAngle; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595638 - Attachment is obsolete: true
(Assignee)

Comment 250

6 years ago
Created attachment 596596 [details] [diff] [review]
part 15 - Remove unnecessary serialisation from setting nsSVGBoolean
(Assignee)

Updated

6 years ago
Attachment #595639 - Attachment is obsolete: true
(Assignee)

Comment 251

6 years ago
Created attachment 596597 [details] [diff] [review]
part 16 - Add mutation event tests for strings and classes and tidy up use of DidChangeString
(Assignee)

Updated

6 years ago
Attachment #595640 - Attachment is obsolete: true
(Assignee)

Comment 252

6 years ago
Created attachment 596598 [details] [diff] [review]
part 17 - Remove unnecessary serialisation from setting nsSVGPreserveAspectRatio; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595641 - Attachment is obsolete: true
(Assignee)

Comment 253

6 years ago
Created attachment 596599 [details] [diff] [review]
part 18 - Remove unnecessary serialisation from setting nsSVGViewBox; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595642 - Attachment is obsolete: true
(Assignee)

Comment 254

6 years ago
Created attachment 596601 [details] [diff] [review]
part 19 - Remove unnecessary serialisation from setting nsSVGNumberList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595643 - Attachment is obsolete: true
(Assignee)

Comment 255

6 years ago
Created attachment 596602 [details] [diff] [review]
part 20 - Remove unnecessary serialisation from setting SVGPointList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595644 - Attachment is obsolete: true
(Assignee)

Comment 256

6 years ago
Created attachment 596604 [details] [diff] [review]
part 21 - Remove unnecessary serialisation from setting SVGPathSegList; r=jwatt
(Assignee)

Updated

6 years ago
Attachment #595645 - Attachment is obsolete: true
(Assignee)

Comment 257

6 years ago
Created attachment 596605 [details] [diff] [review]
part 22 - Remove unnecessary serialisation from setting SVGTransformList; r=jwatt
(Assignee)

Updated

6 years ago