Remove MOZ_SVG conditions in the tree

RESOLVED FIXED in mozilla7

Status

()

Core
General
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mounir, Assigned: emorley)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Severity: normal → minor
Severity: minor → trivial
Flags: in-testsuite-
Whiteboard: [patchlove] [good first bug]
Blocks: 448156
(Assignee)

Comment 1

6 years ago
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
Assignee: nobody → bmo
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Blocks: 597882
(Assignee)

Comment 2

6 years ago
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.
Attachment #535949 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 4

6 years ago
Cool, thanks for the heads up :-)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [patchlove] [good first bug]
(Assignee)

Comment 6

6 years ago
Created attachment 535964 [details] [diff] [review]
Remove MOZ_SVG conditions

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
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Comment 8

6 years ago
(In reply to comment #7)
> I cleaned up the whitespace in browser.js before pushing this.

Thanks! :-)

Comment 9

6 years ago
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

6 years ago
A followup patch/bug is fine we don't need to back this out IMHO.
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Oh, the while loop. *Facepalm*

I'll sort out a followup patch now.
(Assignee)

Comment 13

6 years ago
Created attachment 536040 [details] [diff] [review]
Followup fix to FillInHTMLTooltip
Attachment #536040 - Flags: review?(longsonr)

Updated

6 years ago
Attachment #536040 - Flags: review?(longsonr) → review+
(Assignee)

Updated

6 years ago
Attachment #535964 - Flags: checked-in+
(Assignee)

Updated

6 years ago
Attachment #536040 - Flags: checked-in?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [Please checkin follow-up patch only]
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [Please checkin follow-up patch only] → [fixed in cedar]
(Reporter)

Updated

6 years ago
Attachment #536040 - Flags: checked-in? → checked-in+
http://hg.mozilla.org/mozilla-central/rev/8ccced70c574
Whiteboard: [fixed in cedar]

Updated

6 years ago
Blocks: 660762
(Reporter)

Updated

6 years ago
Attachment #535964 - Flags: checked-in+ → checkin+
(Reporter)

Updated

6 years ago
Attachment #536040 - Flags: checked-in+ → checkin+
You need to log in before you can comment on or make changes to this bug.