Use RAII notifier classes to simplify code

NEW
Assigned to

Status

()

enhancement
P3
normal
4 months ago
2 months ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

4 months ago
No description provided.
Assignee

Comment 1

4 months ago
Posted patch 1524314.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #9040470 - Flags: review?(jwatt)
Assignee

Comment 2

4 months ago
Posted patch 1524314.txtSplinter Review
Attachment #9040470 - Attachment is obsolete: true
Attachment #9040470 - Flags: review?(jwatt)
Attachment #9041330 - Flags: review?(jwatt)
Assignee

Updated

4 months ago
Attachment #9041330 - Attachment is patch: true
Priority: -- → P3
Assignee

Updated

3 months ago
Attachment #9041330 - Flags: review?(jwatt) → review?(dholbert)
Comment on attachment 9041330 [details] [diff] [review]
1524314.txt

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

Sorry for the delay on this. r=me, with nits addressed:

::: dom/svg/SVGAnimatedPreserveAspectRatio.cpp
@@ +42,5 @@
> +// Stack-based helper class to pair calls to WillChangePreserveAspectRatio and
> +// DidChangePreserveAspectRatio.
> +class MOZ_RAII AutoChangePreserveAspectRatioNotifier {
> + public:
> +  explicit AutoChangePreserveAspectRatioNotifier(

Here & in every constructor in this patch, the 'explicit' keyword shouldn't be necessary, so let's remove it.

(You only need that for single-arg constructors, and none of the constructors in this patch are single-arg, I'm pretty sure.)

(Be sure to run ./mach clang-format after making this change, since it may result in some rewrapping/unwrapping due to shorter lines.)

::: dom/svg/SVGInteger.cpp
@@ +23,5 @@
> +// Stack-based helper class ensure DidChangeInteger is called.
> +class MOZ_RAII AutoChangeIntegerNotifier {
> + public:
> +  explicit AutoChangeIntegerNotifier(SVGInteger *aInteger,
> +                                     SVGElement *aSVGElement

move asterisks to the left (type-hugging)

@@ +40,5 @@
> +  }
> +
> + private:
> +  SVGInteger *const mInteger;
> +  SVGElement *const mSVGElement;

move asterisks to the left (type-hugging)

::: dom/svg/SVGViewBox.cpp
@@ +13,4 @@
>  #include "SVGViewBoxSMILType.h"
>  #include "nsTextFormatter.h"
>  
> +using namespace mozilla::dom;

Most of the changes in this file are "dom::" namespacing changes (this 'using' statement, and various "dom::" prefix removals).

Would you mind splitting those out into a separate patch, to keep this already-quite-broad patch focused on just doing one thing (adding RAII classes)?  Feel free to land that patch (with this file's dom:: & "using namespace" changes) as a "part 0" or whatever makes sense here, with rs=me (rubberstamp).

@@ +96,5 @@
> +//----------------------------------------------------------------------
> +// Helper class: AutoChangeViewBoxNotifier
> +// Stack-based helper class to pair calls to WillChangeViewBox and
> +// DidChangeViewBox.
> +class MOZ_RAII AutoChangeViewBoxNotifier {

This whole chunk (the RAII class definition) probably wants to move up ~3 lines, to be before the "Implementation of SVGViewBox methods" comment.

(In mozilla-central, that comment is directly before some SVGViewbox::[...] method implementations, and we should probably keep it that way.)
Attachment #9041330 - Flags: review?(dholbert) → review+
Assignee

Updated

2 months ago
Keywords: leave-open

Comment 4

2 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6838e7d3960f
Part 1 - use using mozilla:dom to reduce typing r=dholbert
Assignee

Updated

2 months ago
Keywords: leave-open

Comment 5

2 months ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1f2814e41d
Part 2 - Use RAII notifier classes to simplify code r=dholbert

Comment 6

2 months ago
Backout by rmaries@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08b9d599f25
Backed out 2 changesets for build bustages. CLOSED TREE

Comment 8

2 months ago

Hi,

I found there is quite a lot of code duplication in these 9 RAII helpers. Especially the 4 helpers for Boolean, Enum, Integer, Number are exactly the same; another 3 helpers IntegerPair, NumberPair, Length are also exactly the same.

Have you considered using template + macro to avoid the duplication?

Assignee

Comment 9

2 months ago

There are at least 3 different variants here, further deduplication could be done but I wouldn't want to make things more confusing. Note that I plan to fix bug 1518666 first now as I think that will fix the windows compilation issue.

Flags: needinfo?(longsonr)

Comment 10

2 months ago

I think the root cause of the code duplication is that we lack some generic interface in methods of dom/svg/SVGElement.h. Every method concerning a specific type has hardcoded the type into the method name. Like SVGElement::DidChangeBoolean, SVGElement::DidChangeInteger, ..., which eliminates the possibility to write generic code.

At least now, the Boolean, Enum, Integer and Number have almost the same interface, but we have to write 4 exact duplicate for them. I think it would help reducing future code duplication if we add some generic method by tag dispatching in SVGElement.

I will attach a prototype patch below.

Flags: needinfo?(longsonr)

Comment 12

2 months ago

There is some typo in the prototype, it should be mSVGElement->template DidChange<tag>(mVal->mAttrEnum); for dependent type. But it's just an idea...

Assignee

Comment 13

2 months ago

Thanks for the ideas Violet. I'll consider incorporating them when I come back to this bug.

Flags: needinfo?(longsonr)
You need to log in before you can comment on or make changes to this bug.