Bug 78547

Created attachment 126856 [details] Patch

Tkent might be curious to see the styling of the datetime inputs, but I think this can just be a rubber-stamp review, it's very simple.

Comment on attachment 126856 [details] Patch > static const RGBA32 defaultTapHighlightColor = 0x6633b5e5; Should this be under "#if ENABLE(TOUCH_EVENTS)" as well?

Comment on attachment 126856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126856&action=review Ok. > Source/WebCore/rendering/RenderThemeChromiumAndroid.h:50 > private: Missing a blank line above this line.

Comment on attachment 126856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126856&action=review > Source/WebCore/WebCore.gyp/WebCore.gyp:825 > '../css/themeChromiumSkia.css', # Chromium only. nit: it seems like someone tried to align them, may be good as a drive-by to remove the extra space here. > Source/WebCore/css/themeChromiumAndroid.css:35 > +select[size][multiple] { Is this last selector necessary? It seems to me that any elements having both @size and @multiple also match one of the earlier two selectors.

Comment on attachment 126856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126856&action=review > Source/WebCore/css/themeChromiumAndroid.css:2 > + * Copyright (C) 2009 Google Inc. All rights reserved. 2012? > Source/WebCore/css/themeChromiumAndroid.css:45 > +input[type="date"], input[type="datetime"], input[type="datetime-local"], input[type="time"], input[type="month"] { > + -webkit-appearance: menulist-button; > +} I understand how they are implemented in Android and it's ok to commit this as is for now. These types inherit a text field type in WebCore/html/*InputType. We should make a better way to switch form control UI. > Source/WebCore/rendering/RenderThemeChromiumAndroid.h:45 > + virtual String extraDefaultStyleSheet(); > > virtual Color systemColor(int cssValidId) const; > > virtual void adjustInnerSpinButtonStyle(CSSStyleSelector*, RenderStyle*, Element*) const; > > + virtual bool delegatesMenuListRendering() const { return true; } > + > +#if ENABLE(TOUCH_EVENTS) > + virtual Color platformTapHighlightColor() const Should append OVERRIDE.

Wow! Thanks for all the reviews! Will upload a new patch.

(In reply to comment #5) > (From update of attachment 126856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126856&action=review > > > Source/WebCore/WebCore.gyp/WebCore.gyp:825 > > '../css/themeChromiumSkia.css', # Chromium only. > > nit: it seems like someone tried to align them, may be good as a drive-by to remove the extra space here. I don't really see any alignment pattern. I can remove the extra space from the chromiumskia one, but I think I'll just leave it: '../css/themeChromium.css', # Chromium only. '../css/themeChromiumAndroid.css', # Chromium only. '../css/themeChromiumLinux.css', # Chromium only. '../css/themeChromiumSkia.css', # Chromium only. > > Source/WebCore/css/themeChromiumAndroid.css:35 > > +select[size][multiple] { > > Is this last selector necessary? It seems to me that any elements having both @size and @multiple also match one of the earlier two selectors. No clue.

Created attachment 127408 [details] Patch for landing

Comment on attachment 127408 [details] Patch for landing Clearing flags on attachment: 127408 Committed r107998: <http://trac.webkit.org/changeset/107998>

All reviewed patches have been landed. Closing bug.