260272 - Clean up nsGlobalChromeWindow::SetCursor()

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Mats Palmgren (inactive)]

Spawned off from bug 163174 comment 95.

If this is the same list, why not use the same code?  (That probably requires
moving nsCSSParser::SearchKeywordTable into nsCSSProps, making it return the
result instead of the index (which wouldn't hurt at all), and then calling
nsCSSKeywords::LookupKeyword and nsCSSProps::SearchKeywordTable (your new one,
which might need a different name from the existing ones, which I think should
be renamed to RSearch* -- and RSearchKeywordTableInt should return an
nsCSSKeyword enum instead of an int).)

Comment 1

[Mats Palmgren (inactive)]

Maybe we can improve (or even remove) nsEventStateManager::SetCursor() too.

Comment 2

[Mats Palmgren (inactive)]

The name "RSearchKeywordTableInt" doesn't say much about its function.
How about "ValueToKeyword" or "GetFirstKeywordWithValue" or something instead?

Comment 3

[Mats Palmgren (inactive)]

Attachment: A hack to use as testcase

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 [Checkin: Comment 5]

Comment 5

[Mats Palmgren (inactive)]

Checked in on trunk 2004-10-01 10:15 PDT

-> FIXED

Comment 6

[Martijn Wargers (dead)]

Attachment: patch for trunk

The 1.9 branch is opened, so this code can now be removed for that branch, not?

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 197083 [details] [diff] [review]
patch for trunk

IIRC, I was thinking of this when writing that comment:
  aCursor.Equals(NS_LITERAL_STRING("auto"))

r=mats either way.

Comment 8

[Mats Palmgren (inactive)]

Attachment: with string fix too

David, feel free to + the one you prefer.

Comment 9

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

Comment on attachment 197091 [details] [diff] [review]
with string fix too

Not sure why this should be after 1.8 -- if you're going to break
compatibility, better to have fewer people relying on it than more.

Please fix:

 * instead of Equals(NS_LITERAL_STRING("auto")), use EqualsLiteral("auto")

 * remove two spaces of indent on the "return NS_OK;", since it's no longer in
an "else".

Comment 10

[Martijn Wargers (dead)]

Mats, the patch has r+ and sr+ so it can be checked in, not?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Mats: are you going to check in any of the followup patches in this bug, or are they not wanted?

Comment 12

[Serge Gautherie (:sgautherie)]

"[e-mail]  	2007-01-29 03:36:52 PST" removed "[checkin needed]".

In an attempt to clarify/answer comment 11, let's summarize:
*The first patch has already been checked in (comment 5).
*The second patch is obsoleted by the third (comments 8 & 9).
*The third patch (was/)is waiting for checkin, with two "nits" (comment 9).

Mats, can you do this ?

Comment 13

[mjh563]

Attachment: with string fix too (updated patch)

It seems that the follow-up patch never got checked in. I've updated it to include the fixes from comment 9.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 748514 [details] [diff] [review]
with string fix too (updated patch)

Thanks.

Comment 15

[mjh563]

I don't have commit access myself, so someone else will need to push it.

Comment 16

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b30b52b30a1

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b30b52b30a1