WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
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-
Details
Formatted Diff
Diff
fix minor formatting nits from earlier patch
(2.83 KB, patch)
2009-09-14 14:54 PDT
,
Dirk Pranke
fishd
: review+
fishd
: commit-queue+
Details
Formatted Diff
Diff
fix comment about tests in changelog
(2.92 KB, patch)
2009-09-14 15:02 PDT
,
Dirk Pranke
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug