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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel.S]

Attachment: 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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: comm-central use [Checkin: Comment 3]

I know better, I swear I do.

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 341848 [details] [diff] [review]
comm-central use
[Checkin: Comment 3]

Pushed: http://hg.mozilla.org/comm-central/rev/22aa09787c8a

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 5

[Daniel.S]

(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?

Comment 6

[Serge Gautherie (:sgautherie)]

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

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(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.

Comment 8

[Serge Gautherie (:sgautherie)]

ToDo: (afaict)
/layout/base/nsPresShell.cpp
/layout/style/nsCSSProps.cpp

Comment 9

[Alfred Kayser]

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

Comment 10

[Daniel.S]

(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

[Alfred Kayser]

As soon as you remove the -moz-outline aliases from nsCSSProps.cpp, the tests will start to fail...

Comment 12

[Daniel.S]

(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.

Comment 13

[Daniel.S]

Attachment: 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)?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's fine to leave them as they are.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 343791 [details] [diff] [review]
nsPresShell.cpp use
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/d90514f93076

Comment 16

[Daniel.S]

Attachment: 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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

r+sr=dbaron

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(But if this doesn't land before beta2, it shouldn't land until the next cycle.)

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should just move this to the next release instead of trying to get it into beta2.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

As per roc's comments, not approved for this release.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Now that there's a third beta, roc/dbaron, should it be considered?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that we want extension authors to develop based on beta2, probably not.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

http://hg.mozilla.org/mozilla-central/rev/f3663178d2e2

Comment 24

[Serge Gautherie (:sgautherie)]

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’
}

Comment 25

[Serge Gautherie (:sgautherie)]

(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 ?

Comment 26

[Serge Gautherie (:sgautherie)]

(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".

Comment 27

[Daniel.S]

(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.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd prefer if you just removed the aliases mechanism entirely now that we're not using it.

Comment 29

[Daniel.S]

Attachment: 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?

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you compile it this time?

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 32

[Daniel.S]

Attachment: 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.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e731e7196ba9

Comment 34

[Serge Gautherie (:sgautherie)]

(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 !

Comment 35

[Daniel.S]

(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 :-)

Comment 36

[Eric Shepherd [:sheppy]]

The documentation has been updated.