Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 846547 - Remove unused arg "aElement" from SVGContentUtils::GetViewboxTransform()
: Remove unused arg "aElement" from SVGContentUtils::GetViewboxTransform()
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Flavio Martins
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2013-02-28 15:33 PST by Daniel Holbert [:dholbert] (PTO Oct 21-25)
Modified: 2013-03-04 14:22 PST (History)
2 users (show)
dholbert: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch to fix Bug 846547 (6.91 KB, patch)
2013-03-01 16:32 PST, maxwellmichelleschmitt
no flags Details | Diff | Splinter Review
Patch for 846547 (8.29 KB, patch)
2013-03-02 16:08 PST, Flavio Martins
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-02-28 15:33:24 PST
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
Comment 1 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-02-28 15:34:34 PST
(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.)
Comment 2 maxwellmichelleschmitt 2013-03-01 14:06:18 PST
I am pretty new to C++, I know my way around a bit. Can you assign me to this bug please?
Comment 3 Robert Longson 2013-03-01 14:29:53 PST
Sure thing. Feel free to let Daniel or me know if you need any help.
Comment 4 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-01 14:32:10 PST
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.
Comment 5 maxwellmichelleschmitt 2013-03-01 14:34:57 PST
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?

Comment 6 maxwellmichelleschmitt 2013-03-01 14:45:11 PST
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)?
Comment 7 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-01 14:58:20 PST
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.
Comment 8 maxwellmichelleschmitt 2013-03-01 14:59:39 PST
Should I drop the "const nsSVGElement*" as well?
Comment 9 maxwellmichelleschmitt 2013-03-01 15:03:00 PST
also in the nsSVGImageFrame.cpp, I see instead of (this) a reference to (element) should that be dropped as (this) was?
Comment 10 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-01 15:23:07 PST
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.)

Comment 11 maxwellmichelleschmitt 2013-03-01 16:32:04 PST
Created attachment 720181 [details] [diff] [review]
Patch to fix Bug 846547
Comment 12 maxwellmichelleschmitt 2013-03-01 16:35:02 PST
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 13 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-01 17:15:14 PST
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!
Comment 14 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-01 17:19:32 PST
(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. :)
Comment 15 Flavio Martins 2013-03-02 16:08:55 PST
Created attachment 720342 [details] [diff] [review]
Patch for 846547
Comment 16 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-03 00:06:30 PST
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!
Comment 17 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-03 00:10:09 PST
(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!)
Comment 18 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-03 00:14:21 PST
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.
Comment 19 maxwellmichelleschmitt 2013-03-03 16:22:58 PST
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?
Comment 20 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2013-03-03 16:41:58 PST
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.
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-04 14:22:40 PST

Note You need to log in before you can comment on or make changes to this bug.