Closed Bug 1524314 Opened 2 years ago Closed 1 day ago

Use RAII notifier classes to simplify code

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch 1524314.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #9040470 - Flags: review?(jwatt)
Attached patch 1524314.txt (obsolete) — Splinter Review
Attachment #9040470 - Attachment is obsolete: true
Attachment #9040470 - Flags: review?(jwatt)
Attachment #9041330 - Flags: review?(jwatt)
Attachment #9041330 - Attachment is patch: true
Priority: -- → P3
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+
Keywords: leave-open
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
Keywords: leave-open
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
Backout by rmaries@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08b9d599f25
Backed out 2 changesets for build bustages. CLOSED TREE

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?

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)

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)

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...

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

Flags: needinfo?(longsonr)
Attachment #9164838 - Attachment is obsolete: true

The compilation issue is fixed by making the LengthList classes into template and then instantiating that so we only have one class of each type.

Attachment #9164838 - Attachment is obsolete: false
Attachment #9041330 - Attachment is obsolete: true
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07a20e5f68e3
Use RAII notifier classes to simplify code r=dholbert
Status: NEW → RESOLVED
Closed: 2 days ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 81 Branch → ---
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd804b4f2f8c
Use RAII notifier classes to simplify code r=dholbert
Status: REOPENED → RESOLVED
Closed: 2 days ago1 day ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
You need to log in before you can comment on or make changes to this bug.