Last Comment Bug 614515 - Remove MOZ_SVG conditions in the tree
: Remove MOZ_SVG conditions in the tree
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
-- trivial (vote)
: mozilla7
Assigned To: Ed Morley [:emorley]
:
:
Mentors:
Depends on: 585020
Blocks: 448156 597882 660762
  Show dependency treegraph
 
Reported: 2010-11-24 02:42 PST by Mounir Lamouri (:mounir)
Modified: 2011-06-06 07:08 PDT (History)
9 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove MOZ_SVG conditions (142.18 KB, patch)
2011-05-29 11:47 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Remove MOZ_SVG conditions (142.25 KB, patch)
2011-05-29 12:56 PDT, Ed Morley [:emorley]
roc: review+
Details | Diff | Splinter Review
Remove MOZ_SVG conditions (142.26 KB, patch)
2011-05-29 15:23 PDT, Ed Morley [:emorley]
emorley: review+
mounir: checkin+
Details | Diff | Splinter Review
Followup fix to FillInHTMLTooltip (2.40 KB, patch)
2011-05-30 02:44 PDT, Ed Morley [:emorley]
longsonr: review+
mounir: checkin+
Details | Diff | Splinter Review

Description User image Mounir Lamouri (:mounir) 2010-11-24 02:42:45 PST
With bug 585020 fixed, all code inside #ifdef MOZ_SVG will always be compiled and all code inside #ifndef MOZ_SVG will never be compiled.
The code inside #ifndef MOZ_SVG conditions should be removed and all #if{n,}def MOZ_SVG conditions.

Bug 585020 contains a patch doing that but has probably to be updated.
Comment 1 User image Ed Morley [:emorley] 2011-05-29 11:47:44 PDT
Created attachment 535949 [details] [diff] [review]
Remove MOZ_SVG conditions

The patch in bug 585020 had rotted quite a lot (100+ failed hunks), so I used what I could and have done the rest from scratch.

Everything should be done, just one question: what should I do with InvalidateInternalAfterResize, here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4207
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#2108
...given that it only existed in case MOZ_SVG wasn't defined, according to the comment. Should I collapse it into InvalidateInternal to optimise further, or just leave as is since it will work fine like that?

Try run (with InvalidateInternalAfterResize left as is for now):
http://dev.philringnalda.com/tbpl/?tree=Try&rev=164a9a9def4b
Comment 2 User image Ed Morley [:emorley] 2011-05-29 12:56:21 PDT
Created attachment 535954 [details] [diff] [review]
Remove MOZ_SVG conditions

Fix missing backslash in layout/build/Makefile.in, doh.

http://dev.philringnalda.com/tbpl/?tree=Try&rev=d99b8a1b1b72

As for the InvalidateInternalAfterResize question in comment 1, ignore it, since it will be dealt with by bug 597882.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2011-05-29 14:14:37 PDT
I probably won't get to this review until next week; probably better to find someone (maybe roc?) who can do it sooner?
Comment 4 User image Ed Morley [:emorley] 2011-05-29 14:25:09 PDT
Cool, thanks for the heads up :-)
Comment 5 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-29 14:41:22 PDT
Comment on attachment 535954 [details] [diff] [review]
Remove MOZ_SVG conditions

Review of attachment 535954 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 6 User image Ed Morley [:emorley] 2011-05-29 15:23:37 PDT
Created attachment 535964 [details] [diff] [review]
Remove MOZ_SVG conditions

Only change is updated commit message to include r=roc.

Carrying forwards r+
Comment 7 User image Dão Gottwald [:dao] 2011-05-30 00:41:30 PDT
http://hg.mozilla.org/mozilla-central/rev/d105fc895d91

I cleaned up the whitespace in browser.js before pushing this.
Comment 8 User image Ed Morley [:emorley] 2011-05-30 01:03:51 PDT
(In reply to comment #7)
> I cleaned up the whitespace in browser.js before pushing this.

Thanks! :-)
Comment 9 User image Robert Longson 2011-05-30 02:18:58 PDT
You've changed the logic of FillInHTMLTooltip

Consider this markup...

<svg><title>boo</title><foreignObject><html><svg><rect/></svg></html></foreignObject></svg>

The rect will now get the title of boo. Before this patch landed it wouldn't as lookingForSVGTitle would become false within the while loop. Whether it's right or wrong to change the logic, I don't think this is the bug to do that in.
Comment 10 User image Robert Longson 2011-05-30 02:22:30 PDT
A followup patch/bug is fine we don't need to back this out IMHO.
Comment 11 User image Ed Morley [:emorley] 2011-05-30 02:26:11 PDT
Thanks for the heads up.

The intention was not to change the logic, but to just remove the now redundant lookingForSVGTitle bool. I'll take a look and see what actually changed.
Comment 12 User image Ed Morley [:emorley] 2011-05-30 02:29:30 PDT
Oh, the while loop. *Facepalm*

I'll sort out a followup patch now.
Comment 13 User image Ed Morley [:emorley] 2011-05-30 02:44:27 PDT
Created attachment 536040 [details] [diff] [review]
Followup fix to FillInHTMLTooltip

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