Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2480993002:
Clean up some NativeTheme code. (Closed)
|
Created:
4 years, 1 month ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up some NativeTheme code.
- improve instantiation
- allow to specify separate instances for web and native
- two NativeThemeAura instances coexist for w/wo overlay scrollbars
BUG=307091
Committed: https://crrev.com/99453a7509f05ca729e66d774f5ae2817c3988c3
Cr-Commit-Position: refs/heads/master@{#430381}
Patch Set 1 #
Total comments: 10
Patch Set 2 : no notreached #
Messages
Total messages: 25 (11 generated)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estade@chromium.org changed reviewers: + bokan@chromium.org, sky@chromium.org
https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); The is_overlay param can't use IsOverlayScrollbarEnabled since that's statically set. Tests can turn overlays on/off dynamically at runtime using Blink's RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch on whether they're using overlays. We should pass in the is_overlay param from client code which can query WebRuntimeFeatures (see my patch). https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); Will GetPartSize ever get called for non-scrollbar parts like SliderThumbs? In that case, we should probably NOTREACHED only on scrollbar parts. https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_dark_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_dark_aura.cc:154: NativeThemeDarkAura::NativeThemeDarkAura() : NativeThemeAura(false) {} The dark theme is used in incognito, right? We should still use overlays there if they're set in non-incognito.
https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); On 2016/11/06 23:37:24, bokan wrote: > The is_overlay param can't use IsOverlayScrollbarEnabled since that's statically > set. Tests can turn overlays on/off dynamically at runtime using Blink's > RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch on > whether they're using overlays. > > We should pass in the is_overlay param from client code which can query > WebRuntimeFeatures (see my patch). can you point me at some of the tests you're talking about? I don't really want to pass any parameters from blink, and it seems like the tests should be able to append command line switches to toggle this setting (if we need to, we can prepend this fn with: if (!IsOverlayScrollbarEnabled()) return instance(); https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); On 2016/11/06 23:37:24, bokan wrote: > Will GetPartSize ever get called for non-scrollbar parts like SliderThumbs? In > that case, we should probably NOTREACHED only on scrollbar parts. probably https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_dark_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_dark_aura.cc:154: NativeThemeDarkAura::NativeThemeDarkAura() : NativeThemeAura(false) {} On 2016/11/06 23:37:24, bokan wrote: > The dark theme is used in incognito, right? We should still use overlays there > if they're set in non-incognito. it's never used by blink
https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); On 2016/11/07 00:40:43, Evan Stade wrote: > On 2016/11/06 23:37:24, bokan wrote: > > The is_overlay param can't use IsOverlayScrollbarEnabled since that's > statically > > set. Tests can turn overlays on/off dynamically at runtime using Blink's > > RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch on > > whether they're using overlays. > > > > We should pass in the is_overlay param from client code which can query > > WebRuntimeFeatures (see my patch). > > can you point me at some of the tests you're talking about? I don't really want > to pass any parameters from blink, and it seems like the tests should be able to > append command line switches to toggle this setting (if we need to, we can > prepend this fn with: > > if (!IsOverlayScrollbarEnabled()) > return instance(); Hm, didn't think about adding command line flags. If we can do that then this is fine. The failing tests are: content_browsertests: RenderViewImplTest.ImeComposition RenderViewImplTest.OnSetTextDirection webkit_unit_tests (though the right way to fix these will be to make them use a mock scrollbar theme, I'll do that in my enabling patch): BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces BaseAudioContextTest.AutoplayMetrics_CreateNoGesture BaseAudioContextTest.AutoplayMetrics_CreateGesture BaseAudioContextTest.AutoplayMetrics_NodeStartGesture BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess BaseAudioContextTest.AutoplayMetrics_CallResumeGesture BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture browser_tests: ThreatDOMDetailsTest.Everything FormClassifierTest.SignupFormWithSigninButtonHTML The following browser_tests also fail but IIRC they're all hitting the DCHECKs from Views so should be fixed by this patch already: TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0 PhishingDOMFeatureExtractorTest.SubFrames TaskManagerBrowserTest.IdleWakeups TaskManagerBrowserTest.WebWorkerJSHeapMemory TaskManagerBrowserTest.JSHeapMemory ScrollbarTest.LongPromptScrollbar TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_dark_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_dark_aura.cc:154: NativeThemeDarkAura::NativeThemeDarkAura() : NativeThemeAura(false) {} On 2016/11/07 00:40:43, Evan Stade wrote: > On 2016/11/06 23:37:24, bokan wrote: > > The dark theme is used in incognito, right? We should still use overlays there > > if they're set in non-incognito. > > it's never used by blink Acknowledged.
https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); On 2016/11/07 12:28:11, bokan wrote: > On 2016/11/07 00:40:43, Evan Stade wrote: > > On 2016/11/06 23:37:24, bokan wrote: > > > The is_overlay param can't use IsOverlayScrollbarEnabled since that's > > statically > > > set. Tests can turn overlays on/off dynamically at runtime using Blink's > > > RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch on > > > whether they're using overlays. > > > > > > We should pass in the is_overlay param from client code which can query > > > WebRuntimeFeatures (see my patch). > > > > can you point me at some of the tests you're talking about? I don't really > want > > to pass any parameters from blink, and it seems like the tests should be able > to > > append command line switches to toggle this setting (if we need to, we can > > prepend this fn with: > > > > if (!IsOverlayScrollbarEnabled()) > > return instance(); > > Hm, didn't think about adding command line flags. If we can do that then this is > fine. This seems like the right approach: https://codereview.chromium.org/2480903002/diff/100001/content/browser/site_p... > The failing tests are: > > content_browsertests: > > RenderViewImplTest.ImeComposition > RenderViewImplTest.OnSetTextDirection I can't get either of these to fail with this patch, with or without flipping the default for overlay scrollbars in native_theme_switches.cc. > > webkit_unit_tests (though the right way to fix these will be to make them use a > mock scrollbar theme, I'll do that in my enabling patch): > > BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces > BaseAudioContextTest.AutoplayMetrics_CreateNoGesture > BaseAudioContextTest.AutoplayMetrics_CreateGesture > BaseAudioContextTest.AutoplayMetrics_NodeStartGesture > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess > BaseAudioContextTest.AutoplayMetrics_CallResumeGesture > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture > BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture > > browser_tests: > > ThreatDOMDetailsTest.Everything > FormClassifierTest.SignupFormWithSigninButtonHTML same, not seeing these fail with this patch (even with scrollbars enabled) > > The following browser_tests also fail but IIRC they're all hitting the DCHECKs > from Views so should be fixed by this patch already: > > TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0 > PhishingDOMFeatureExtractorTest.SubFrames > TaskManagerBrowserTest.IdleWakeups > TaskManagerBrowserTest.WebWorkerJSHeapMemory > TaskManagerBrowserTest.JSHeapMemory > ScrollbarTest.LongPromptScrollbar > TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); On 2016/11/07 00:40:43, Evan Stade wrote: > On 2016/11/06 23:37:24, bokan wrote: > > Will GetPartSize ever get called for non-scrollbar parts like SliderThumbs? In > > that case, we should probably NOTREACHED only on scrollbar parts. > > probably restored this (minus some of the comment about views layout). Other parts are still hit. Stack trace: http://hastebin.com/raw/acuxamocor
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/07 15:05:57, Evan Stade wrote: > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > File ui/native_theme/native_theme_aura.cc (right): > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); > On 2016/11/07 12:28:11, bokan wrote: > > On 2016/11/07 00:40:43, Evan Stade wrote: > > > On 2016/11/06 23:37:24, bokan wrote: > > > > The is_overlay param can't use IsOverlayScrollbarEnabled since that's > > > statically > > > > set. Tests can turn overlays on/off dynamically at runtime using Blink's > > > > RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch > on > > > > whether they're using overlays. > > > > > > > > We should pass in the is_overlay param from client code which can query > > > > WebRuntimeFeatures (see my patch). > > > > > > can you point me at some of the tests you're talking about? I don't really > > want > > > to pass any parameters from blink, and it seems like the tests should be > able > > to > > > append command line switches to toggle this setting (if we need to, we can > > > prepend this fn with: > > > > > > if (!IsOverlayScrollbarEnabled()) > > > return instance(); > > > > Hm, didn't think about adding command line flags. If we can do that then this > is > > fine. > > This seems like the right approach: > https://codereview.chromium.org/2480903002/diff/100001/content/browser/site_p... > > > The failing tests are: > > > > content_browsertests: > > > > RenderViewImplTest.ImeComposition > > RenderViewImplTest.OnSetTextDirection > > I can't get either of these to fail with this patch, with or without flipping > the default for overlay scrollbars in native_theme_switches.cc. > > > > > webkit_unit_tests (though the right way to fix these will be to make them use > a > > mock scrollbar theme, I'll do that in my enabling patch): > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces > > BaseAudioContextTest.AutoplayMetrics_CreateNoGesture > > BaseAudioContextTest.AutoplayMetrics_CreateGesture > > BaseAudioContextTest.AutoplayMetrics_NodeStartGesture > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess > > BaseAudioContextTest.AutoplayMetrics_CallResumeGesture > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture > > BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture > > > > browser_tests: > > > > ThreatDOMDetailsTest.Everything > > FormClassifierTest.SignupFormWithSigninButtonHTML > > same, not seeing these fail with this patch (even with scrollbars enabled) > > > > > The following browser_tests also fail but IIRC they're all hitting the DCHECKs > > from Views so should be fixed by this patch already: > > > > TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0 > > PhishingDOMFeatureExtractorTest.SubFrames > > TaskManagerBrowserTest.IdleWakeups > > TaskManagerBrowserTest.WebWorkerJSHeapMemory > > TaskManagerBrowserTest.JSHeapMemory > > ScrollbarTest.LongPromptScrollbar > > TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); > On 2016/11/07 00:40:43, Evan Stade wrote: > > On 2016/11/06 23:37:24, bokan wrote: > > > Will GetPartSize ever get called for non-scrollbar parts like SliderThumbs? > In > > > that case, we should probably NOTREACHED only on scrollbar parts. > > > > probably > > restored this (minus some of the comment about views layout). Other parts are > still hit. Stack trace: http://hastebin.com/raw/acuxamocor Ok, this is lgtm then. I'm compiling with this patch applied now to look into tests and the NOTREACHED hits coming from Blink.
On 2016/11/07 16:48:50, bokan wrote: > On 2016/11/07 15:05:57, Evan Stade wrote: > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > File ui/native_theme/native_theme_aura.cc (right): > > > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); > > On 2016/11/07 12:28:11, bokan wrote: > > > On 2016/11/07 00:40:43, Evan Stade wrote: > > > > On 2016/11/06 23:37:24, bokan wrote: > > > > > The is_overlay param can't use IsOverlayScrollbarEnabled since that's > > > > statically > > > > > set. Tests can turn overlays on/off dynamically at runtime using Blink's > > > > > RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch > > on > > > > > whether they're using overlays. > > > > > > > > > > We should pass in the is_overlay param from client code which can query > > > > > WebRuntimeFeatures (see my patch). > > > > > > > > can you point me at some of the tests you're talking about? I don't really > > > want > > > > to pass any parameters from blink, and it seems like the tests should be > > able > > > to > > > > append command line switches to toggle this setting (if we need to, we can > > > > prepend this fn with: > > > > > > > > if (!IsOverlayScrollbarEnabled()) > > > > return instance(); > > > > > > Hm, didn't think about adding command line flags. If we can do that then > this > > is > > > fine. > > > > This seems like the right approach: > > > https://codereview.chromium.org/2480903002/diff/100001/content/browser/site_p... > > > > > The failing tests are: > > > > > > content_browsertests: > > > > > > RenderViewImplTest.ImeComposition > > > RenderViewImplTest.OnSetTextDirection > > > > I can't get either of these to fail with this patch, with or without flipping > > the default for overlay scrollbars in native_theme_switches.cc. > > > > > > > > webkit_unit_tests (though the right way to fix these will be to make them > use > > a > > > mock scrollbar theme, I'll do that in my enabling patch): > > > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces > > > BaseAudioContextTest.AutoplayMetrics_CreateNoGesture > > > BaseAudioContextTest.AutoplayMetrics_CreateGesture > > > BaseAudioContextTest.AutoplayMetrics_NodeStartGesture > > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess > > > BaseAudioContextTest.AutoplayMetrics_CallResumeGesture > > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture > > > BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture > > > > > > browser_tests: > > > > > > ThreatDOMDetailsTest.Everything > > > FormClassifierTest.SignupFormWithSigninButtonHTML > > > > same, not seeing these fail with this patch (even with scrollbars enabled) > > > > > > > > The following browser_tests also fail but IIRC they're all hitting the > DCHECKs > > > from Views so should be fixed by this patch already: > > > > > > TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0 > > > PhishingDOMFeatureExtractorTest.SubFrames > > > TaskManagerBrowserTest.IdleWakeups > > > TaskManagerBrowserTest.WebWorkerJSHeapMemory > > > TaskManagerBrowserTest.JSHeapMemory > > > ScrollbarTest.LongPromptScrollbar > > > TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory > > > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); > > On 2016/11/07 00:40:43, Evan Stade wrote: > > > On 2016/11/06 23:37:24, bokan wrote: > > > > Will GetPartSize ever get called for non-scrollbar parts like > SliderThumbs? > > In > > > > that case, we should probably NOTREACHED only on scrollbar parts. > > > > > > probably > > > > restored this (minus some of the comment about views layout). Other parts are > > still hit. Stack trace: http://hastebin.com/raw/acuxamocor > > Ok, this is lgtm then. I'm compiling with this patch applied now to look into > tests and the NOTREACHED hits coming from Blink. I'm able to reproduce the failures in: ThreatDOMDetailsTest.Everything FormClassifierTest.SignupFormWithSigninButtonHTML and RenderViewImplTest.ImeComposition RenderViewImplTest.OnSetTextDirection By building for ChromeOS (from linux) and turning on the flag for ChromeOS but I can address those with my "enable flag" CL by passing kDisableOverlayScrollbars.
On 2016/11/07 17:03:36, bokan wrote: > On 2016/11/07 16:48:50, bokan wrote: > > On 2016/11/07 15:05:57, Evan Stade wrote: > > > > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > > File ui/native_theme/native_theme_aura.cc (right): > > > > > > > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > > ui/native_theme/native_theme_aura.cc:70: (IsOverlayScrollbarEnabled())); > > > On 2016/11/07 12:28:11, bokan wrote: > > > > On 2016/11/07 00:40:43, Evan Stade wrote: > > > > > On 2016/11/06 23:37:24, bokan wrote: > > > > > > The is_overlay param can't use IsOverlayScrollbarEnabled since that's > > > > > statically > > > > > > set. Tests can turn overlays on/off dynamically at runtime using > Blink's > > > > > > RuntimeEnabledFeatures in which case Blink and NativeTheme will > mismatch > > > on > > > > > > whether they're using overlays. > > > > > > > > > > > > We should pass in the is_overlay param from client code which can > query > > > > > > WebRuntimeFeatures (see my patch). > > > > > > > > > > can you point me at some of the tests you're talking about? I don't > really > > > > want > > > > > to pass any parameters from blink, and it seems like the tests should be > > > able > > > > to > > > > > append command line switches to toggle this setting (if we need to, we > can > > > > > prepend this fn with: > > > > > > > > > > if (!IsOverlayScrollbarEnabled()) > > > > > return instance(); > > > > > > > > Hm, didn't think about adding command line flags. If we can do that then > > this > > > is > > > > fine. > > > > > > This seems like the right approach: > > > > > > https://codereview.chromium.org/2480903002/diff/100001/content/browser/site_p... > > > > > > > The failing tests are: > > > > > > > > content_browsertests: > > > > > > > > RenderViewImplTest.ImeComposition > > > > RenderViewImplTest.OnSetTextDirection > > > > > > I can't get either of these to fail with this patch, with or without > flipping > > > the default for overlay scrollbars in native_theme_switches.cc. > > > > > > > > > > > webkit_unit_tests (though the right way to fix these will be to make them > > use > > > a > > > > mock scrollbar theme, I'll do that in my enabling patch): > > > > > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces > > > > BaseAudioContextTest.AutoplayMetrics_CreateNoGesture > > > > BaseAudioContextTest.AutoplayMetrics_CreateGesture > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartGesture > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess > > > > BaseAudioContextTest.AutoplayMetrics_CallResumeGesture > > > > BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture > > > > BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture > > > > > > > > browser_tests: > > > > > > > > ThreatDOMDetailsTest.Everything > > > > FormClassifierTest.SignupFormWithSigninButtonHTML > > > > > > same, not seeing these fail with this patch (even with scrollbars enabled) > > > > > > > > > > > The following browser_tests also fail but IIRC they're all hitting the > > DCHECKs > > > > from Views so should be fixed by this patch already: > > > > > > > > > TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0 > > > > PhishingDOMFeatureExtractorTest.SubFrames > > > > TaskManagerBrowserTest.IdleWakeups > > > > TaskManagerBrowserTest.WebWorkerJSHeapMemory > > > > TaskManagerBrowserTest.JSHeapMemory > > > > ScrollbarTest.LongPromptScrollbar > > > > TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory > > > > > > > > > https://codereview.chromium.org/2480993002/diff/1/ui/native_theme/native_them... > > > ui/native_theme/native_theme_aura.cc:269: NOTREACHED(); > > > On 2016/11/07 00:40:43, Evan Stade wrote: > > > > On 2016/11/06 23:37:24, bokan wrote: > > > > > Will GetPartSize ever get called for non-scrollbar parts like > > SliderThumbs? > > > In > > > > > that case, we should probably NOTREACHED only on scrollbar parts. > > > > > > > > probably > > > > > > restored this (minus some of the comment about views layout). Other parts > are > > > still hit. Stack trace: http://hastebin.com/raw/acuxamocor > > > > Ok, this is lgtm then. I'm compiling with this patch applied now to look into > > tests and the NOTREACHED hits coming from Blink. > > I'm able to reproduce the failures in: > > ThreatDOMDetailsTest.Everything > FormClassifierTest.SignupFormWithSigninButtonHTML > > and > > RenderViewImplTest.ImeComposition > RenderViewImplTest.OnSetTextDirection > > By building for ChromeOS (from linux) and turning on the flag for ChromeOS but I > can address those with my "enable flag" CL by passing kDisableOverlayScrollbars. well, that's odd, as it's the same configuration I tried.
p.s. sky, you technically only need to review ui/views/view.cc, but feel free to review more if you care to.
LGTM
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Clean up some NativeTheme code. - improve instantiation - allow to specify separate instances for web and native - two NativeThemeAura instances coexist for w/wo overlay scrollbars BUG=307091 ========== to ========== Clean up some NativeTheme code. - improve instantiation - allow to specify separate instances for web and native - two NativeThemeAura instances coexist for w/wo overlay scrollbars BUG=307091 Committed: https://crrev.com/99453a7509f05ca729e66d774f5ae2817c3988c3 Cr-Commit-Position: refs/heads/master@{#430381} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/99453a7509f05ca729e66d774f5ae2817c3988c3 Cr-Commit-Position: refs/heads/master@{#430381}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2484043003/ by scheib@chromium.org. The reason for reverting is: Scroll bars are appearing as errors in layout tests: e.g. fast/forms/select/listbox-appearance-basic.html https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fa... This rollback is speculative, but there are few possibilities in the blame list https://chromium.googlesource.com/chromium/src/+log/40639df0c3a5e05761a846b2f.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
