Bug 29239 – RenderThemeChromiumWin has inconsistent values for state and classic_state

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dirk Pranke]

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).

[Dirk Pranke]

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 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]

It will be tested by a Chromium patch that will land after this lands.

[Peter Kasting]

FWIW the changes seem OK to me other than adding some whitespace on an other-wise blank line.

[Dirk Pranke]

Created attachment 39567 [details]
patch to determineClassicState() to fix the {part,state}->classic_state mapping

[Darin Fisher (:fishd, Google)]

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 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]

Created attachment 39573 [details]
fix minor formatting nits from earlier patch

[WebKit Commit Bot]

Comment on attachment 39573 [details]
fix minor formatting nits from earlier patch

Rejecting patch 39573 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Dirk Pranke]

Created attachment 39574 [details]
fix comment about tests in changelog

[Eric Seidel (no email)]

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]

(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 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]

All reviewed patches have been landed.  Closing bug.