Closed Bug 614515 Opened 14 years ago Closed 13 years ago

Remove MOZ_SVG conditions in the tree

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla7

People

(Reporter: mounir, Assigned: emorley)

References

Details

Attachments

(2 files, 2 obsolete files)

Remove MOZ_SVG conditions
142.26 KB, patch
emorley
: review+
mounir
: checkin+
Details | Diff | Splinter Review
Followup fix to FillInHTMLTooltip
2.40 KB, patch
longsonr
: review+
mounir
: checkin+
Details | Diff | Splinter Review 
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.
Severity: normal → minor
Severity: minor → trivial
Flags: in-testsuite-
Whiteboard: [patchlove] [good first bug]
Attached patch Remove MOZ_SVG conditions (obsolete) — — Splinter Review 
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
Assignee: nobody → bmo
Status: NEW → ASSIGNED
Blocks: 597882
Attached patch Remove MOZ_SVG conditions (obsolete) — — Splinter Review 
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.
Attachment #535949 - Attachment is obsolete: true
Attachment #535954 - Flags: review?(dbaron)
I probably won't get to this review until next week; probably better to find someone (maybe roc?) who can do it sooner?
Cool, thanks for the heads up :-)
Attachment #535954 - Flags: review?(dbaron) → review?(roc)
Comment on attachment 535954 [details] [diff] [review]
Remove MOZ_SVG conditions

Review of attachment 535954 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #535954 - Flags: review?(roc) → review+
Keywords: checkin-needed
Whiteboard: [patchlove] [good first bug]
Only change is updated commit message to include r=roc.

Carrying forwards r+
Attachment #535954 - Attachment is obsolete: true
Attachment #535964 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/d105fc895d91

I cleaned up the whitespace in browser.js before pushing this.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(In reply to comment #7)
> I cleaned up the whitespace in browser.js before pushing this.

Thanks! :-)
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.
A followup patch/bug is fine we don't need to back this out IMHO.
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.
Oh, the while loop. *Facepalm*

I'll sort out a followup patch now.

        Attachment #536040 -
        Flags: review?(longsonr)

        Attachment #536040 -
        Flags: review?(longsonr) → review+

    
  

        Attachment #535964 -
        Flags: checked-in+

    
  

        Attachment #536040 -
        Flags: checked-in?

    
  
Keywords: checkin-needed
Whiteboard: [Please checkin follow-up patch only]

    
  
Keywords: checkin-needed
Whiteboard: [Please checkin follow-up patch only] → [fixed in cedar]

    
  

        Attachment #536040 -
        Flags: checked-in? → checked-in+

    
  
Blocks: 660762

    
  

        Attachment #535964 -
        Flags: checked-in+ → checkin+

    
  

        Attachment #536040 -
        Flags: checked-in+ → checkin+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: