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 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 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 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 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 Ed Morley [:emorley] 2011-05-29 14:25:09 PDT
Cool, thanks for the heads up :-)
Comment 5 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 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 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 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 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 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 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 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 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.