scrollbar arrow code abuses look and feel system

ASSIGNED

Status

()

Core
XUL
ASSIGNED
12 years ago
11 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
The look and feel system is hardcoded to return fixed values for the eMetric_ScrollButtonXXXMouseButtonAction values; on GTK2 they are always 0, 1 and 2 while on other platforms they are always 0, 3 and 3. Either the hardcoded values should be moved to nsScrollbarButtonFrame.cpp or the metrics should be fixed to use the look and feel system correctly.
(Assignee)

Comment 1

12 years ago
Created attachment 230421 [details] [diff] [review]
hardcode values

Not shown are the backouts of the changes to look and feel.
Attachment #230421 - Flags: review?(ispiked)
(Assignee)

Comment 2

12 years ago
Created attachment 230427 [details] [diff] [review]
correct usage
Attachment #230427 - Flags: review?(ispiked)

Updated

12 years ago
Attachment #230421 - Flags: review?(ispiked)

Updated

12 years ago
Attachment #230427 - Flags: review?(ispiked)

Comment 3

12 years ago
I really can't review these changes. FWIW, roc was the one who advised me to make these changes in order to fix the win32 bustage.

As roc mentioned, we really don't want to allow people to change these prefs. so I think that using the ifdefs for GTK2 would be OK here.
(Assignee)

Comment 4

12 years ago
As I don't have GTK2 could you at least test that the patch you prefer works?
(Assignee)

Comment 5

12 years ago
Created attachment 230472 [details] [diff] [review]
hardcode values
Assignee: jag → neil
Attachment #230421 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #230472 - Flags: superreview?(roc)
Attachment #230472 - Flags: review?(roc)
I don't know why you feel that hardcoding execution paths with #ifdefs is preferable to using nsILookAndFeel. I think it's much more ugly and less extensible. The difference in complexity is marginal.
In particular, with the "hardcoded values" patch, if we ever want to support any other behaviour for any button on any other platform, the code will become a complete mess.
(Assignee)

Comment 8

12 years ago
(In reply to comment #7)
>In particular, with the "hardcoded values" patch, if we ever want to support
>any other behaviour for any button on any other platform, the code will become
>a complete mess.
Well I guess I could write it slightly differently:
#ifdef MOZ_WIDGET_GTK2
#define LEFTBUTTONACTION pressedButtonAction = 0
#define MIDDLEBUTTONACTION pressedButtonAction = 1
#define RIGHTBUTTONACTION pressedButtonAction = 2
#else
#define LEFTBUTTONACTION pressedButtonAction = 0
#define MIDDLEBUTTONACTION return PR_FALSE
#define RIGHTBUTTONACTION return PR_FALSE
#endif
and leave most of the existing scrollbarbutton code in place.
I don't think that's an improvement, honestly. And I don't know why you find using nsILookAndFeel so upsetting here.
(Assignee)

Comment 10

12 years ago
(In reply to comment #9)
>I don't know why you find using nsILookAndFeel so upsetting here.
Because those particular metrics don't work like any of the existing metrics.
nsILookAndFeel is an interface. The new settings fit nicely into it. The fact that they're implemented differently shouldn't matter, in my opinion.
Attachment #230472 - Flags: superreview?(roc)
Attachment #230472 - Flags: superreview-
Attachment #230472 - Flags: review?(roc)
Attachment #230472 - Flags: review-
You need to log in before you can comment on or make changes to this bug.