The default bug view has changed. See this FAQ.

remove uses of -moz-outline* and drop the aliases afterwards

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Daniel.S, Assigned: Daniel.S)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.2a1
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 341808 [details] [diff] [review]
s/-moz-outline-offset/outline-offset/ for mozilla-central
[Checkin: Comment 6]

See Bug 413141 comment #13 and #14.

The patch replaces several uses of -moz-outline-offset with outline-offset.

There are also some reftests that use both. Should they stay as they are or should I include them in the patch?
Attachment #341808 - Flags: superreview?(bzbarsky)
Attachment #341808 - Flags: review?(myk)
Comment on attachment 341808 [details] [diff] [review]
s/-moz-outline-offset/outline-offset/ for mozilla-central
[Checkin: Comment 6]

If the reftests use both, leave them as they are, I would say.
Attachment #341808 - Flags: superreview?(bzbarsky) → superreview+
Created attachment 341848 [details] [diff] [review]
comm-central use
[Checkin: Comment 3]

I know better, I swear I do.
Attachment #341848 - Flags: review?(mkmelin+mozilla)

Updated

9 years ago
Attachment #341848 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 341848 [details] [diff] [review]
comm-central use
[Checkin: Comment 3]

Pushed: http://hg.mozilla.org/comm-central/rev/22aa09787c8a
Attachment #341848 - Attachment description: comm-central use → [checked in] comm-central use
Comment on attachment 341808 [details] [diff] [review]
s/-moz-outline-offset/outline-offset/ for mozilla-central
[Checkin: Comment 6]

This is an obvious good fix, but note that I'm not a toolkit peer, so I'm not authorized to review its code.
Attachment #341808 - Flags: review?(myk) → review+
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)
> (From update of attachment 341808 [details] [diff] [review])
> This is an obvious good fix, but note that I'm not a toolkit peer, so I'm not
> authorized to review its code.

Thanks. Should I ask for another review then, or am I fine with r+sr?
(Assignee)

Updated

9 years ago
Attachment #341808 - Flags: review?(enndeakin)
Attachment #341808 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 341808 [details] [diff] [review]
s/-moz-outline-offset/outline-offset/ for mozilla-central
[Checkin: Comment 6]

http://hg.mozilla.org/mozilla-central/rev/1965b824bb06
Attachment #341808 - Attachment description: s/-moz-outline-offset/outline-offset/ for mozilla-central → s/-moz-outline-offset/outline-offset/ for mozilla-central [Checkin: Comment 6]
Attachment #341848 - Attachment description: [checked in] comm-central use → comm-central use [Checkin: Comment 3]
(In reply to comment #5)
> Thanks. Should I ask for another review then, or am I fine with r+sr?

Erm, sorry, I wasn't cc:ed on this bug, so I didn't get this question.  You did the right thing to ask for another review.
ToDo: (afaict)
/layout/base/nsPresShell.cpp
/layout/style/nsCSSProps.cpp
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed

Comment 9

9 years ago
You also want to remove the -moz-outline-* (except for radius) from the tests (reftest and layout/test).
(Assignee)

Comment 10

9 years ago
(In reply to comment #7)
> Erm, sorry, I wasn't cc:ed on this bug, so I didn't get this question.  You did
> the right thing to ask for another review.

No problem, it was really my fault. I'm still new to patch writing after all.

(In reply to comment #9)
> You also want to remove the -moz-outline-* (except for radius) from the tests
> (reftest and layout/test).

In comment #1, Boris suggested otherwise, so now I'm really unsure what to do about them.

Comment 11

9 years ago
As soon as you remove the -moz-outline aliases from nsCSSProps.cpp, the tests will start to fail...
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> As soon as you remove the -moz-outline aliases from nsCSSProps.cpp, the tests
> will start to fail...

Actually, they won't fail. All the tests got both, -moz-outline-offset and outline-offset. That's why I initially asked wether to remove the prefixed properties or not.

Personally I'm fine with any choice, but I'd prefer not do something someone would much rather I'm not doing.
(Assignee)

Comment 13

9 years ago
Created attachment 343791 [details] [diff] [review]
nsPresShell.cpp use
[Checkin: Comment 15]

This removes the last occurrence of -moz-outline-offset in nsPresShell.cpp and the whole mozilla-central.

The reftests can be changed any time in the future.

btw, roc, do you have an opinion on wether the reftests should use both: -moz-outline-offset and outline-offset (that's the current situation) or only the latter (proposed by Alfred)?
Attachment #343791 - Flags: superreview?(roc)
Attachment #343791 - Flags: review?(roc)
Attachment #343791 - Flags: superreview?(roc)
Attachment #343791 - Flags: superreview+
Attachment #343791 - Flags: review?(roc)
Attachment #343791 - Flags: review+
I think it's fine to leave them as they are.
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 343791 [details] [diff] [review]
nsPresShell.cpp use
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/d90514f93076
Attachment #343791 - Attachment description: nsPresShell.cpp use → nsPresShell.cpp use [Checkin: Comment 15]
Keywords: checkin-needed
(Assignee)

Comment 16

9 years ago
Created attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

This is a simple patch that empties the gAliases array. That's probably the best idea because new aliases may be needed in the future.

The one question I got is wether the empty string is correct in the sizeof() part.
Attachment #345721 - Flags: superreview?(dbaron)
Attachment #345721 - Flags: review?(dbaron)
Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

r+sr=dbaron
Attachment #345721 - Flags: superreview?(dbaron)
Attachment #345721 - Flags: superreview+
Attachment #345721 - Flags: review?(dbaron)
Attachment #345721 - Flags: review+
(But if this doesn't land before beta2, it shouldn't land until the next cycle.)
Keywords: checkin-needed

Updated

9 years ago
Attachment #345721 - Flags: approval1.9.1b2?

Updated

9 years ago
Keywords: checkin-needed
I think we should just move this to the next release instead of trying to get it into beta2.
Attachment #345721 - Flags: approval1.9.1b2? → approval1.9.1b2-
Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

As per roc's comments, not approved for this release.
Now that there's a third beta, roc/dbaron, should it be considered?
Keywords: checkin-needed
Given that we want extension authors to develop based on beta2, probably not.
Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

http://hg.mozilla.org/mozilla-central/rev/f3663178d2e2
Attachment #345721 - Attachment description: remove the aliases → remove the aliases [Checkin: Comment 23]
Attachment #345721 - Attachment description: remove the aliases [Checkin: Comment 23] → remove the aliases [Backout: Comment 24]
Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/19e40cd74411
http://hg.mozilla.org/mozilla-central/rev/c9d7a1d3f413

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228568571.1228569116.26944.gz
Linux mozilla-central build on 2008/12/06 05:02:51

/builds/moz2_slave/mozilla-central-linux/build/layout/style/nsCSSProps.cpp:149: error: zero-size array ‘gAliases’
}
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
Attachment #345721 - Attachment is obsolete: true
(In reply to comment #24)

Windows too:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228569640.1228570044.29013.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/12/06 05:20:40

e:/slave/trunk_win2k3-7/build/layout/style/nsCSSProps.cpp(149) : error C2466: cannot allocate an array of constant size 0
}

Where did you succeed to compile this patch ?
(In reply to comment #8)
> ToDo: (afaict)

I confirm that the only things now left in tree are:
*"-moz-outline-radius*".
*a few tests with both "-moz-outline-offset" and "outline-offset".
(Assignee)

Comment 27

8 years ago
(In reply to comment #25)
> Where did you succeed to compile this patch ?

Actually I didn't try because this was supposed to be a small change. I'm terribly sorry.

Hm, so we need to approach this in a different way.
I could leave -moz-outline-offset in, because it's CSS3 (CR though), but that seems to be inconsequent.

(In reply to comment #26)
> I confirm that the only things now left in tree are:
> *"-moz-outline-radius*".

These are no aliases (and defined in no standard as far as I know).

> *a few tests with both "-moz-outline-offset" and "outline-offset".

We decided to leave them as they are for now.
I'd prefer if you just removed the aliases mechanism entirely now that we're not using it.
(Assignee)

Comment 29

8 years ago
Created attachment 351707 [details] [diff] [review]
no more aliases

(In reply to comment #28)
> I'd prefer if you just removed the aliases mechanism entirely now that we're
> not using it.

ok, that's it, or is it?
Attachment #351707 - Flags: superreview?(dbaron)
Attachment #351707 - Flags: review?(dbaron)
Did you compile it this time?
Comment on attachment 351707 [details] [diff] [review]
no more aliases

>-  // LookupProperty(nsACString&).  The table will do its own
>+  // LookupProperty(nsACString&). The table will do its own

The spacing was better the way it was.

r+sr=dbaron if you compiled it
Attachment #351707 - Flags: superreview?(dbaron)
Attachment #351707 - Flags: superreview+
Attachment #351707 - Flags: review?(dbaron)
Attachment #351707 - Flags: review+
(Assignee)

Comment 32

8 years ago
Created attachment 351749 [details] [diff] [review]
patch for checkin

(In reply to comment #31)
> r+sr=dbaron if you compiled it

Not personally, but Bill Gianopoulos was so kind to include the patch in his builds. They work fine after my tests. (The next patch will be built by my own hands again).

> The spacing was better the way it was.

Here's the same patch that preserves the spacing.
Attachment #351707 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e731e7196ba9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(In reply to comment #27)
> Actually I didn't try because this was supposed to be a small change. I'm
> terribly sorry.

No big harm this time,
but patches are required to have been compiled ... to help prevent any such "unexpected things" :-|

> > *"-moz-outline-radius*".
> These are no aliases (and defined in no standard as far as I know).
> > *a few tests with both "-moz-outline-offset" and "outline-offset".
> We decided to leave them as they are for now.

I wasn't complaining: I was confirming that there was no more leftovers, this time :-)

Thanks for working on this bug !
(Assignee)

Comment 35

8 years ago
(In reply to comment #34)
> No big harm this time,
> but patches are required to have been compiled ... to help prevent any such
> "unexpected things" :-|

I'll definitely keep this in mind.

> I wasn't complaining: I was confirming that there was no more leftovers, this
> time :-)

All right, I won't forget this either!

I'll let the bug rest now :-)
The documentation has been updated.
Keywords: dev-doc-complete
Depends on: 521503
You need to log in before you can comment on or make changes to this bug.