| Summary: | [Chromium] Multi-value select boxes cannot do non-contiguous selection | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robert Sesek <rsesek> | ||||||||||
| Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | darin, eric, fishd, playmobil, sam | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| URL: | http://code.google.com/p/chromium/issues/detail?id=15998 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Robert Sesek
2009-08-23 13:58:32 PDT
Created attachment 38458 [details]
Fix
Comment on attachment 38458 [details]
Fix
WebKit convention is to cc relevant parties and not to assign specific reviewers, thus I'm moving Darin to the cc list.
This does not seem like a Darwin level idea, it is very specific to the Mac. Is this how we have been solving other cases similar to this? We use at the top of the file: #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) I spoke to Jeremy about hit and he thought we could reduce it to just PLATFORM(DARWIN). (In reply to comment #4) > We use at the top of the file: > > #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) > > I spoke to Jeremy about hit and he thought we could reduce it to just > PLATFORM(DARWIN). Mechanically that's OK, but it's not exactly the right concept. PLATFORM(DARWIN) is really supposed to be an OS-level switch like PLATFORM(WINOS) that tells you what low level OS is on the target machine. PLATFORM(DARWIN) means Apple's Unix variant with things like frameworks and certain libraries expected to be installed. This seems to be more about which style of user interface we want. Chromium creates a non-Mac platform that wants to follow Mac user interface style because while it's virtual it is on a Mac. To make that clear I think we'll probably need a different macro in the long run. Short run, we might be OK saying PLATFORM(DARWIN) here unless there's some other port running with PLATFORM(DARWIN) defined that doesn't want this behavior. For example, it's not too farfetched to imagine trouble if you were compiling for QT or GTK on a Mac. (In reply to comment #5) > Mechanically that's OK, but it's not exactly the right concept. > PLATFORM(DARWIN) is really supposed to be an OS-level switch like > PLATFORM(WINOS) that tells you what low level OS is on the target machine. > PLATFORM(DARWIN) means Apple's Unix variant with things like frameworks and > certain libraries expected to be installed. > > This seems to be more about which style of user interface we want. Chromium > creates a non-Mac platform that wants to follow Mac user interface style > because while it's virtual it is on a Mac. > > To make that clear I think we'll probably need a different macro in the long > run. Short run, we might be OK saying PLATFORM(DARWIN) here unless there's some > other port running with PLATFORM(DARWIN) defined that doesn't want this > behavior. For example, it's not too farfetched to imagine trouble if you were > compiling for QT or GTK on a Mac. Then to be on the safe side, I think the answer is to use, like we do earlier in the same file, PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)). From what I've discovered on mailing lists, PLATFORM(MAC) is used pretty much for Safari on OS X. Perhaps the right answer long-term is to make a PLATFORM(SAFARI) so that PLATFORM(MAC) can be used in instances like this. Created attachment 38478 [details] Adjusted condition per comment 6 (In reply to comment #6) > From what I've discovered on mailing lists, PLATFORM(MAC) is used pretty much > for Safari on OS X. Perhaps the right answer long-term is to make a > PLATFORM(SAFARI) so that PLATFORM(MAC) can be used in instances like this. This is a great oversimplification and would not be correct. For example, Safari works on Windows. And tons of Mac OS X applications use the PLATFORM(MAC) version of WebKit, not just Safari. PLATFORM(MAC) is about having WebKit on top of Mac OS X frameworks like AppKit. The Chromium port is peculiar because it's WebKit on Mac OS X, but striving to not be on top of something like AppKit because of the particular architecture of the Chrome browser, and instead of being on top of the OS frameworks, it's on top a special Chromium layer. We do have a plan to fix the platform macros and make various more specific names, which should solve this problem. Maciej Stachowiak sent mail to webkit-dev a couple months ago about it. But PLATFORM(SAFARI) would be terrible and a step in the wrong direction! Robert: The discussion of the right macros to use for cases like this seems to come up every time we need some code to be used on both Chrome/OS X & Safari since the current situation is sub-optimal. I think in the long term we need to fix this but I think the discussion of what that solution is belongs in a separate bug or on webkit-dev. Per-Darin's comment could you resubmit your first patch and use PLATFORM(DARWIN). Sam: Up till now we've resolved these issues by changing these to PLATFORM(DARWIN). Darin & Sam: Thanks for your comments, I don't want to sound like we don't care about doing the right thing for this, we most certain do want a good long term solution here! But as mentioned above, I think we should discuss this in another scope. Created attachment 38489 [details]
Using just PLATFORM(DARWIN)
Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)
I think this change looks fine. However some might disagree with me, given how contentious and mis-understood the PLATFORM #defines have been in the pass. I really hope that Maciej delivers his grand re-write soon to end all this needless confusion. :)
Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)
Rejecting patch 38489 from commit-queue. This patch will require manual commit.
Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: None
Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)
commit-queue bug. checkin occured to the ChangeLog file while the commit-queue was processing this.
PLATFORM(DARWIN) really isn't the right macro to be using here. The desired is specific to the behavior of the Mac OS X interface. It may not make sense for an X11 application on Mac OS X, or directly on top of Darwin. Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)
Rejecting patch 38489 from commit-queue. This patch will require manual commit.
Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: None
Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)
Clearing r+ since this is not correct.
Mark: What do you think the right #ifdef is here? Here is what we have been using thus far: #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) If that is not correct, we should discuss it separately, and come up with a better solution to apply globally. (In reply to comment #18) > Here is what we have been using thus far: > > #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) > > If that is not correct, we should discuss it separately, and come up with a > better solution to apply globally. While not perfect, this seems more reasonable to me. Comment on attachment 38478 [details] Adjusted condition per comment 6 > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 47695) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,20 @@ > +2009-08-23 Robert Sesek <rsesek@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + [Chromium] Multi-value select boxes cannot do non-contiguous selection on Mac > + https://bugs.webkit.org/show_bug.cgi?id=28670 > + > + Change the condition regarding the key modifier check for non-contiguous > + selection on a multi-value select from PLATFORM(MAC) to PLATFORM(DARWIN), so > + that Chromium works as expected on Mac. This doesn't describe the change that was actually made, so it'll need to be updated. The actual code change looks fine. r=me Created attachment 38514 [details]
FINAL- Revised Changelog
Thanks, Mark. I've added a new patch with the revised Changelog.
Is the final patch supposed to be marked for review? Comment on attachment 38514 [details]
FINAL- Revised Changelog
Oops, you're right. Flag set, now.
Comment on attachment 38478 [details] Adjusted condition per comment 6 obsoleting this patch since a new one has been posted. Comment on attachment 38514 [details]
FINAL- Revised Changelog
LGTM.
Comment on attachment 38514 [details] FINAL- Revised Changelog Clearing flags on attachment: 38514 Committed r47777: <http://trac.webkit.org/changeset/47777> All reviewed patches have been landed. Closing bug. |