RESOLVED FIXED 29239
RenderThemeChromiumWin has inconsistent values for state and classic_state
https://bugs.webkit.org/show_bug.cgi?id=29239
Summary RenderThemeChromiumWin has inconsistent values for state and classic_state
Dirk Pranke
Reported 2009-09-13 22:33:29 PDT
Parts of RenderThemeChromiumWin do not properly check the states and values of the controls when determining the value of the 'classic_state' parameter (which is passed to WebThemeEngine). As a result, for a given part/state combination, we can often get multiple classic_state values (which are, it turns out, ignored in the native_theme implementation).
Attachments
patch to determineClassicState() to fix the {part,state}->classic_state mapping (2.77 KB, patch)
2009-09-13 22:38 PDT, Dirk Pranke
eric: commit-queue-
patch to determineClassicState() to fix the {part,state}->classic_state mapping (2.83 KB, patch)
2009-09-14 13:01 PDT, Dirk Pranke
fishd: review+
eric: commit-queue-
fix minor formatting nits from earlier patch (2.83 KB, patch)
2009-09-14 14:54 PDT, Dirk Pranke
fishd: review+
fishd: commit-queue+
fix comment about tests in changelog (2.92 KB, patch)
2009-09-14 15:02 PDT, Dirk Pranke
no flags
Dirk Pranke
Comment 1 2009-09-13 22:38:56 PDT
Created attachment 39534 [details] patch to determineClassicState() to fix the {part,state}->classic_state mapping I would also have Peter review this, but I'm not sure of his email address ...
Eric Seidel (no email)
Comment 2 2009-09-14 09:59:40 PDT
Comment on attachment 39534 [details] patch to determineClassicState() to fix the {part,state}->classic_state mapping Please don't change the NOBODY (OOPS!) from the review line otherwise svn-apply won't know how to set the reviewer (and thus it can't be landed via the commit queue). Can this be tested? Generally we require either tests, or explanation of why tests are impossible/impractical in the ChangeLog.
Dirk Pranke
Comment 3 2009-09-14 12:43:47 PDT
It will be tested by a Chromium patch that will land after this lands.
Peter Kasting
Comment 4 2009-09-14 12:44:55 PDT
FWIW the changes seem OK to me other than adding some whitespace on an other-wise blank line.
Dirk Pranke
Comment 5 2009-09-14 13:01:31 PDT
Created attachment 39567 [details] patch to determineClassicState() to fix the {part,state}->classic_state mapping
Darin Fisher (:fishd, Google)
Comment 6 2009-09-14 14:49:03 PDT
Comment on attachment 39567 [details] patch to determineClassicState() to fix the {part,state}->classic_state mapping nit: should only have one space before '//' but LGTM otherwise.
Eric Seidel (no email)
Comment 7 2009-09-14 14:53:20 PDT
Comment on attachment 39567 [details] patch to determineClassicState() to fix the {part,state}->classic_state mapping 10 No new tests. (OOPS!) Will fail to land. That should be replaced by either a mention of the new tests you added or an explanation of why testing is impossible/impractical. In this case, I expect this patch affects tests but the results live only in Chromium's tree? In that case, you should mention that, including which tests it affects (if it's a small number). Marking cq- since commit will fail with that OOPS there. The other OOPS on the review line is automatically fixed by svn-apply during the commit process.
Dirk Pranke
Comment 8 2009-09-14 14:54:54 PDT
Created attachment 39573 [details] fix minor formatting nits from earlier patch
WebKit Commit Bot
Comment 9 2009-09-14 14:56:00 PDT
Comment on attachment 39573 [details] fix minor formatting nits from earlier patch Rejecting patch 39573 from review queue. dpranke@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Dirk Pranke
Comment 10 2009-09-14 15:02:36 PDT
Created attachment 39574 [details] fix comment about tests in changelog
Eric Seidel (no email)
Comment 11 2009-09-14 15:05:08 PDT
Comment on attachment 39574 [details] fix comment about tests in changelog The attachment isn't marked as a patch. :) You might find "bugzilla-tool post-diff 29239" useful for posting patches. It handles things like marking it for review, auto-obsoleting old patches, making sure new attachments are attached with the correct mime type, etc.
Dirk Pranke
Comment 12 2009-09-14 15:23:27 PDT
(In reply to comment #11) > (From update of attachment 39574 [details]) > The attachment isn't marked as a patch. :) > You might find "bugzilla-tool post-diff 29239" useful for posting patches. > It handles things like marking it for review, auto-obsoleting old patches, > making sure new attachments are attached with the correct mime type, etc. Good to know; I'll use that in the future. Thanks!
WebKit Commit Bot
Comment 13 2009-09-14 15:38:45 PDT
Comment on attachment 39574 [details] fix comment about tests in changelog Clearing flags on attachment: 39574 Committed r48371: <http://trac.webkit.org/changeset/48371>
WebKit Commit Bot
Comment 14 2009-09-14 15:38:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.