Bug 73916 - Drop ENABLE_NO_LISTBOX_RENDERING, and make it a runtime decision.
Summary: Drop ENABLE_NO_LISTBOX_RENDERING, and make it a runtime decision.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-06 05:55 PST by Pierre Rossi
Modified: 2011-12-08 19:22 PST (History)
4 users (show)

See Also:

Attachments
Patch (14.54 KB, patch)
2011-12-06 06:42 PST, Pierre Rossi 		no flags Details | Formatted Diff | Diff
Patch (14.55 KB, patch)
2011-12-07 06:18 PST, Pierre Rossi 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2011-12-06 05:55:37 PST 
Drop ENABLE_NO_LISTBOX_RENDERING, and make it a runtime decision.
Comment 1 Pierre Rossi 2011-12-06 06:42:44 PST 
Created attachment 118040 [details]
Patch
Comment 2 Kent Tamura 2011-12-06 18:13:10 PST 
Comment on attachment 118040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118040&action=review

Thank you for working on this!

> Source/WebCore/html/HTMLSelectElement.cpp:187
> +    Page* page = 0;
> +    if (Frame* frame = document()->frame())
> +        page = frame->page();

We have Document::page().

> Source/WebCore/html/HTMLSelectElement.cpp:188
> +    RenderTheme* renderTheme = page ? page->theme() : RenderTheme::defaultTheme().leakRef();

Do not leak.
This should be RefPtr<RenterTheme> renderTheme = ...

> Source/WebCore/rendering/RenderTheme.h:210
> +    virtual bool usesMenuList() const { return false; }

The function name should be more descriptive.  selectElementAlwaysUsesMenuList() or something?
Comment 3 Pierre Rossi 2011-12-07 06:18:21 PST 
Created attachment 118203 [details]
Patch

@Kent: thanks for taking a look at this. Any idea what render themes in chromium would need to switch to this ? I failed to track the define all the way.
Comment 4 Kent Tamura 2011-12-07 19:26:58 PST 
(In reply to comment #3)
> @Kent: thanks for taking a look at this. Any idea what render themes in chromium would need to switch to this ? I failed to track the define all the way.

I think Android port is using ENABLE_NO_LISTBOX_RENDERING, but RenderTheme for Android is not upstreamed yet.  So you don't need to care about it.
Comment 5 Kent Tamura 2011-12-07 19:27:35 PST 
Comment on attachment 118203 [details]
Patch

Looks good.
Comment 6 WebKit Review Bot 2011-12-08 04:54:49 PST 
Comment on attachment 118203 [details]
Patch

Rejecting attachment 118203 [details] from commit-queue.

pierre.rossi@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 7 WebKit Review Bot 2011-12-08 19:22:35 PST 
Comment on attachment 118203 [details]
Patch

Clearing flags on attachment: 118203

Committed r102419: <http://trac.webkit.org/changeset/102419>
Comment 8 WebKit Review Bot 2011-12-08 19:22:40 PST 
All reviewed patches have been landed.  Closing bug.