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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Serge Gautherie (:sgautherie) 2008-10-17 15:58:02 PDT
ToDo: (afaict)
/layout/base/nsPresShell.cpp
/layout/style/nsCSSProps.cpp
Comment 9 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image 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 User image 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 User image 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 User image David Baron :dbaron: ⌚️UTC-8 2008-11-26 20:39:42 PST
Given that we want extension authors to develop based on beta2, probably not.
Comment 23 User image 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 User image 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 User image 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 User image 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 User image 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image 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 User image David Baron :dbaron: ⌚️UTC-8 2008-12-06 14:09:22 PST
Did you compile it this time?
Comment 31 User image David Baron :dbaron: ⌚️UTC-8 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 User image 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 User image Dão Gottwald [:dao] 2008-12-07 05:37:58 PST
http://hg.mozilla.org/mozilla-central/rev/e731e7196ba9
Comment 34 User image 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 User image 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 User image 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.