Closed Bug 846547 Opened 7 years ago Closed 7 years ago

Remove unused arg "aElement" from SVGContentUtils::GetViewboxTransform()


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: xhaker)


(Whiteboard: [mentor=dholbert][lang=c++])


(1 file, 1 obsolete file)

The two versions of SVGContentUtils::GetViewBoxTransform(), defined here...
...both take an argument "aElement", which they don't use.

We should just drop that argument, and fix the callers to no longer pass it so that we'll still compile.

SO: We need to drop "aElement" from the function-signatures in the .h file here:

...and we need to drop it from the definition and all the calls here:^[^\0]*%24&hitlimit=&tree=mozilla-central
(In reply to Daniel Holbert [:dholbert] from comment #0)
> SO: We need to drop "aElement" from the function-signatures in the .h file
> here:

(Just drop it from the GetViewBoxTransform() function signatures, of course -- not from other methods in that header file.)
I am pretty new to C++, I know my way around a bit. Can you assign me to this bug please?
Sure thing. Feel free to let Daniel or me know if you need any help.
Assignee: nobody → maxwellmichelleschmitt
If you haven't yet built firefox yourself, you can start here...
...and continue on to the "Simple Firefox build (Get_the_source)" link at the bottom there.
I downloaded the source, and have modified it. This line for example: )const nsSVGElement* aElement) became (const nsSVGElement*). Is this correct? How do I submit a patch? At what stage should the code be built?

My only real problem is that when I went to remove it from the calls, I noticed that the entire element (const nsSVGElement* aElement) was replaced by (this). Does this mean that I cannot simply remove the aElement from the statement because this actually refers to the full statement (const nsSVGElement* aElement)?
To be clear:
 - "const nsSVGElement*" is the type of the argument.
 - aElement is the local variable-name.
 - Any callers that pass "this" are instances of nsSVGElement, so their "this" pointer is of type "nsSVGElement*"

Anyway -- you can just drop "this, " from those calls and all should be well.
Should I drop the "const nsSVGElement*" as well?
also in the nsSVGImageFrame.cpp, I see instead of (this) a reference to (element) should that be dropped as (this) was?
Sorry, I didn't see comment 5 until just now.

To answer it (and comment 8):
 "const nsSVGElement* aElement"
is the full variable-declaration for the paramater "aElement".

To drop "aElement", you need to drop that whole string of text -- the type _and_ the variable-name.  And yeah, in nsImageFrame, you should drop "element" from the call.

To submit a patch, follow the directions here: 

And you should verify that the code compiles before you do that.

(BTW: You might want to learn a bit more about C++ before tackling other Mozilla C++ bugs.  Your code-contributions are very much appreciated, but I think you'd be more effective with a bit more background-knowledge than you seem to currently have. :)  This is a good bug for you to start on, though, because it doesn't require too much C++ knowledge.)

Attached patch Patch to fix Bug 846547 (obsolete) — Splinter Review
Uploaded! I do not believe any other files were changed other than the ones requested. Though I would appreciate it if you made sure it actually runs, and I didn't mess a file up or something :) . Thank you for the advice, I needed it :) .
Now, if this gets accepted I can apply for some sort of commit access? Is there a link that explains the different levels?

Comment on attachment 720181 [details] [diff] [review]
Patch to fix Bug 846547

># HG changeset patch
># Parent 993d7aff3109aa96712b198b3ad51a66b006330b
># User Maxwell Schmitt <>
>diff --git a/content/svg/content/src/SVGContentUtils.cpp b/content/svg/content/src/SVGContentUtils.cpp

Looks like this needs a commit message.  If you've applied your patch (with "hg qpush"), you should be able to edit the patch-file's commit message by running "hg qref -e" -- that should pop up an editor where you can type in a message.

The commit message should look something like this:
Bug 846547: Remove unused argument "aElement" from SVGContentUtils::GetViewBoxTransform. r=dholbert

(r=dholbert indicates that I've reviewed it -- that's not quite true yet, but it *will* be true by the time someone checks it in, so it's safe to stick it in the commit message pre-emptively if you like)

One note on the code:
> gfxMatrix
>-SVGContentUtils::GetViewBoxTransform(const nsSVGElement* aElement,
>                                      float aViewportWidth, float aViewportHeight,

For lines like this, could you remove that empty-newline, and make it look like:
  SVGContentUtils::GetViewBoxTransform(float aViewportWidth, float aViewportHeight,

No need to introduce all that blank space. :) 

>+      SVGContentUtils::GetViewBoxTransform(
>                                            viewportWidth, viewportHeight,

Same here -- this should look something like: 
        SVGContentUtils::GetViewBoxTransform(viewportWidth, viewportHeight,
This applies to basically every chunk of this patch.

Also: I tried to compile with this patch, and it didn't compile, due to an invocation of GetViewBoxTransform in nsSVGPatternFrame.cpp that needs to have this fix applied to it.  (It's the final entry in my MXR link at the end of comment 0)

You really should try to compile the patch yourself locally, if possible.  Without a build setup, it'll be hard for you to effectively write Firefox patches. :)

So: I'd suggest that you add a commit message, fix that newline issue, fix the nsSVGPatternFrame chunk, and try to compile, as noted above.  After that, please post your updated patch, and when you're posting it, tick the "review" dropdown to "?" and type in "" into the textbox that appears, and I'll sign off on it.  Thanks!
(In reply to maxwellmichelleschmitt from comment #12)
> Now, if this gets accepted I can apply for some sort of commit access? Is
> there a link that explains the different levels? describes the commit access policy.

Basically: Full commit access takes a while to earn, largely because you don't actually need it on order to get your code committed.  If you post a patch on a bug and it gets reviewed by an appropriate reviewer, you can request to have it checked in, and someone will come along and check it in for you within a day or so in most cases.

Most people don't get commit access until they've contributed a fairly large amount of code -- so, be patient. :)
Attached patch Patch for 846547Splinter Review
Attachment #720342 - Flags: review?(dholbert)
Comment on attachment 720342 [details] [diff] [review]
Patch for 846547

Flavio: Just as a heads-up: generally, it's best to avoid and posting patches on a bug that someone else is already assigned to & working on, to avoid duplicating work & stepping on toes.  (or at least, to ask before doing so)

However, in this case, since this bug is relatively trivial (and the patch is just code-removal), I'm not too concerned, especially since your patch is correct and ready-to-land. (nice!)

So -- r=me. I'll check this in tomorrow & switch the assignee over to Flavio.

Thanks to both Flavio & Maxwell for your work here!
Attachment #720342 - Flags: review?(dholbert) → review+
Assignee: maxwellmichelleschmitt → xhaker
(Maxwell: I hope you're not too put off by having this bug co-opted. ;)  Hopefully this helped you get off the ground in terms of learning how to write a patch & perhaps compile, for the benefit of future bugs that you work on. Thanks again!)
I just went ahead and checked this in:

The next time mozilla-inbound is merged to mozilla-central (within a day or two), whoever does that merge will close this bug.
Flags: in-testsuite-
Sorry I couldn't respond. Been in full day seminars for the weekend. May I still upload my final patch so I may get access to the try servers?
You seem very intent on getting commit access. :)  I think it'd be a bit premature at this point, even for Try server.  Maybe fix a few more bugs first? In the meantime, others will be more than willing to push other patches to Try for you, if you need something tested on other platforms.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.