Closed Bug 1450643 Opened 2 years ago Closed 2 years ago

Simplify code to get/set attributes

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(2 files, 1 obsolete file)

There's a SetAttr that does not require a namespace in Element.h but no GetAttr or HasAttr.

SVGAnimationElement seems to have some pointless wrappers around GetAttr
Attached patch 1450643.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8964250 - Flags: review?(dholbert)
Comment on attachment 8964250 [details] [diff] [review]
1450643.txt

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

::: dom/base/Element.h
@@ +1586,5 @@
> +   * Get an attribute in the simplest way possible.
> +   */
> +  bool GetAttr(nsAtom* aName, nsAString& aResult) const
> +  {
> +    return GetAttr(kNameSpaceID_None, aName, aResult);

I would defer to a DOM reviewer on this change.  Two nits, though:
 - These should probably be declared alongside the functions that they're simplifications of -- around line 734.
 - Documentation should be a bit clearer (probably refer implicitly or explicitly to the to the other methods' documentation, and should mention "without any namespace" or something like that, assuming that's the only difference.)

Also: assuming we do take this change, it seems like we should have a single dedicated patch just for this part (adding these accessors and dropping kNameSpaceID_None from all the callsites to make them use these new accessors).

::: dom/html/nsGenericHTMLElement.h
@@ +788,4 @@
>    }
>    void SetHTMLAttr(nsAtom* aName, const nsAString& aValue, mozilla::ErrorResult& aError)
>    {
> +    SetAttr(aName, aValue, aError);

I would defer to DOM folks on this file's changes, too.

(It seems fine, though I'm wondering if perhaps the namespacing there is just to provide clarity for human readers, so they know which source file to go to, to find the definition of the code we're calling here...  Personally, I can never keep track of which functions live in nsINode vs. nsIContent vs. Element, so maybe this namespacing is a hint-to-the-reader to help disambiguate?)
(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #2)
> I would defer to a DOM reviewer on this change.  Two nits, though:
>  - These should probably be declared alongside the functions that they're
> simplifications of -- around line 734.
>  - Documentation should be a bit clearer (probably refer implicitly or
> explicitly to the to the other methods' documentation, and should mention
> "without any namespace" or something like that, assuming that's the only
> difference.)

Oh, I see you're cribbing the existing location / documentation from the SetAttr method.

Still: I think that location/documentation is silly, and you shouldn't follow that bad example. :)  These seem like they'd be much better alongside the methods that they're wrapping. (And you could perhaps docuemnt them by just extending the existing methods' documentation to have:
   * @param aNameSpaceID the namespace of the attribute (assumed to be
                         kNameSpaceID_None in the version that omits this arg)
(In reply to Robert Longson [:longsonr] from comment #0)
> SVGAnimationElement seems to have some pointless wrappers around GetAttr

This simplification seems good, though as noted in comment 2, I think it should be split into a "part 2" patch, with part 1 being purely about Element::Get/HasAttr (and its direct callsites) getting an implied kNameSpaceID_None.
Attachment #8964250 - Flags: review?(dholbert) → review-
Attachment #8964250 - Attachment is obsolete: true
Attachment #8964390 - Flags: review?(nika)
Attachment #8964391 - Flags: review?(dholbert)
Attachment #8964390 - Attachment description: address review comments → part 1 - address review comments
Attachment #8964391 - Attachment description: address review comments → part 2 - address review comments
Comment on attachment 8964391 [details] [diff] [review]
part 2 - address review comments

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

Thanks! r=me, assuming a sane commit message (doesn't look like patch includes a commit message right now)
Attachment #8964391 - Flags: review?(dholbert) → review+
patch 1 - Add additional methods to Element for GetAttr/HasAttr for kNameSpaceID_None to complement the existing SetAttr method that does not take a namespace.

patch 2 - remove SVGAnimationElement wrappers around GetAttr/HasAttr in favour of directly using the Element versions.
Sounds good to me!
Comment on attachment 8964390 [details] [diff] [review]
part 1 - address review comments

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

::: dom/base/Element.h
@@ +723,5 @@
>     * Get the current value of the attribute. This returns a form that is
>     * suitable for passing back into SetAttr.
>     *
> +   * @param aNameSpaceID the namespace of the attr (assumed to be
> +                         kNameSpaceID_None in the version that omits this arg)

comment nit: s/version/overload please.

Also can we have it say "defaults to" rather than "assumed to be"?

@@ +742,5 @@
>    /**
>     * Determine if an attribute has been set (empty string or otherwise).
>     *
> +   * @param aNameSpaceId the namespace id of the attribute (assumed to be
> +                         kNameSpaceID_None in the version that omits this arg)

same here
Attachment #8964390 - Flags: review?(nika) → review+
Keywords: leave-open
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe50f8117503
part 1 - add additional overloads to Element for GetAttr/HasAttr for kNameSpaceID_None to complement the existing SetAttr method that does not take a namespace. r=mystor
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1965d28a52
part 2 - remove SVGAnimationElement wrappers around GetAttr/HasAttr in favour of directly using the Element versions. r=dholbert
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/fe50f8117503
https://hg.mozilla.org/mozilla-central/rev/cf1965d28a52
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.