Last Comment Bug 458588 - remove uses of -moz-outline* and drop the aliases afterwards
: remove uses of -moz-outline* and drop the aliases afterwards
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Daniel.S
:
: Jet Villegas (:jet)
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 521503
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-05 05:42 PDT by Daniel.S
Modified: 2009-10-09 16:54 PDT (History)
10 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
s/-moz-outline-offset/outline-offset/ for mozilla-central [Checkin: Comment 6] (3.95 KB, patch)
2008-10-05 05:42 PDT, Daniel.S
myk: review+
gavin.sharp: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
comm-central use [Checkin: Comment 3] (642 bytes, patch)
2008-10-05 14:18 PDT, Phil Ringnalda (:philor)
mkmelin+mozilla: review+
Details | Diff | Splinter Review
nsPresShell.cpp use [Checkin: Comment 15] (1.33 KB, patch)
2008-10-19 02:41 PDT, Daniel.S
roc: review+
roc: superreview+
Details | Diff | Splinter Review
remove the aliases [Backout: Comment 24] (1012 bytes, patch)
2008-10-31 08:57 PDT, Daniel.S
dbaron: review+
dbaron: superreview+
mbeltzner: approval1.9.1b2-
Details | Diff | Splinter Review
no more aliases (2.45 KB, patch)
2008-12-06 12:00 PST, Daniel.S
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
patch for checkin (2.39 KB, patch)
2008-12-07 01:14 PST, Daniel.S
no flags Details | Diff | Splinter Review

Description Daniel.S 2008-10-05 05:42:55 PDT
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?
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2008-10-05 12:04:50 PDT
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) 2008-10-05 14:18:59 PDT
Created attachment 341848 [details] [diff] [review]
comm-central use
[Checkin: Comment 3]

I know better, I swear I do.
Comment 3 Phil Ringnalda (:philor) 2008-10-06 14:57:49 PDT
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] 2008-10-08 14:59:46 PDT
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 2008-10-10 06:55:34 PDT
(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) 2008-10-17 15:47:21 PDT
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] 2008-10-17 15:48:50 PDT
(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) 2008-10-17 15:58:02 PDT
ToDo: (afaict)
/layout/base/nsPresShell.cpp
/layout/style/nsCSSProps.cpp
Comment 9 Alfred Kayser 2008-10-18 00:11:58 PDT
You also want to remove the -moz-outline-* (except for radius) from the tests (reftest and layout/test).
Comment 10 Daniel.S 2008-10-18 00:16:14 PDT
(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 2008-10-18 04:22:44 PDT
As soon as you remove the -moz-outline aliases from nsCSSProps.cpp, the tests will start to fail...
Comment 12 Daniel.S 2008-10-18 04:59:28 PDT
(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 2008-10-19 02:41:45 PDT
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)?
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-10-19 21:41:48 PDT
I think it's fine to leave them as they are.
Comment 15 Serge Gautherie (:sgautherie) 2008-10-28 22:12:31 PDT
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 2008-10-31 08:57:54 PDT
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.
Comment 17 David Baron :dbaron: ⌚️UTC-10 2008-11-03 16:56:01 PST
Comment on attachment 345721 [details] [diff] [review]
remove the aliases
[Backout: Comment 24]

r+sr=dbaron
Comment 18 David Baron :dbaron: ⌚️UTC-10 2008-11-03 16:57:08 PST
(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) 2008-11-11 11:56:27 PST
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] 2008-11-11 20:59:47 PST
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] 2008-11-26 20:15:21 PST
Now that there's a third beta, roc/dbaron, should it be considered?
Comment 22 David Baron :dbaron: ⌚️UTC-10 2008-11-26 20:39:42 PST
Given that we want extension authors to develop based on beta2, probably not.
Comment 23 Serge Gautherie (:sgautherie) 2008-12-06 05:01:01 PST
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) 2008-12-06 05:28:58 PST
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) 2008-12-06 05:37:28 PST
(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) 2008-12-06 06:09:33 PST
(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 2008-12-06 07:14:39 PST
(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-10 2008-12-06 09:00:09 PST
I'd prefer if you just removed the aliases mechanism entirely now that we're not using it.
Comment 29 Daniel.S 2008-12-06 12:00:47 PST
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?
Comment 30 David Baron :dbaron: ⌚️UTC-10 2008-12-06 14:09:22 PST
Did you compile it this time?
Comment 31 David Baron :dbaron: ⌚️UTC-10 2008-12-06 14:12:29 PST
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 2008-12-07 01:14:28 PST
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.
Comment 33 Dão Gottwald [:dao] 2008-12-07 05:37:58 PST
http://hg.mozilla.org/mozilla-central/rev/e731e7196ba9
Comment 34 Serge Gautherie (:sgautherie) 2008-12-07 06:01:59 PST
(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 2008-12-07 06:49:35 PST
(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] 2009-02-11 12:53:26 PST
The documentation has been updated.

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