Bug 76424

Created attachment 123269 [details] Patch

Comment on attachment 123269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123269&action=review > Source/WebCore/ChangeLog:12 > + [Refactoring][Internals] Should have InternalSettings > + https://bugs.webkit.org/show_bug.cgi?id=76424 > + > + Reviewed by NOBODY (OOPS!). > + > + - Invoked Internals::reset() in the constructor to employ Document object. > + - Moved setting and configuration related Internals methods to > + newly introduced InternalSettings object. > + > + No new tests, covered by existing tests. The main visible change of this patch is to introduce window.internals.settings. But the ChangeLog doesn't mention it. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:20947 > 41815C1F138319830057AAA4 /* WebCoreTestSupport.h in Headers */, > + A7BF7EE014C9175A0014489D /* InternalSettings.h in Headers */, > + A740B5A514C935AB00A77FA4 /* JSInternalSettings.h in Headers */, We had better sort lines. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:24401 > 417DA71D13735DFA007C57FB /* JSInternals.cpp in Sources */, > 41815C1E138319830057AAA4 /* WebCoreTestSupport.cpp in Sources */, > 07230CBC14C10ED900F6B702 /* JSInternalsCustom.cpp in Sources */, > + A7BF7EDF14C9175A0014489D /* InternalSettings.cpp in Sources */, > + A740B5A714C935AF00A77FA4 /* JSInternalSettings.cpp in Sources */, ditto. > Source/WebCore/testing/InternalSettings.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2012 > Source/WebCore/testing/InternalSettings.cpp:128 > +void InternalSettings::setForceCompositingMode(bool enabled, ExceptionCode& ec) > +{ > + if (!settings()) { > + ec = INVALID_ACCESS_ERR; > + return; > + } > + > + settings()->setForceCompositingMode(enabled); > +} > + > +void InternalSettings::setEnableCompositingForFixedPosition(bool enabled, ExceptionCode& ec) > +{ > + if (!settings()) { > + ec = INVALID_ACCESS_ERR; > + return; > + } > + > + settings()->setAcceleratedCompositingForFixedPositionEnabled(enabled); > +} Repeating very similar implementations is not good. I think it's ok to use a preprocessor macro to generate functions.

Comment on attachment 123269 [details] Patch Attachment 123269 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11297039

Comment on attachment 123269 [details] Patch Attachment 123269 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11297048

Created attachment 123283 [details] Patch

Comment on attachment 123269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123269&action=review Hi Kent-san, thanks for your responsive feedback! I updated the patch to address it. >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20947 >> + A740B5A514C935AB00A77FA4 /* JSInternalSettings.h in Headers */, > > We had better sort lines. Done. >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:24401 >> + A740B5A714C935AF00A77FA4 /* JSInternalSettings.cpp in Sources */, > > ditto. Done. >> Source/WebCore/testing/InternalSettings.cpp:2 >> + * Copyright (C) 2011 Google Inc. All rights reserved. > > 2012 Done. >> Source/WebCore/testing/InternalSettings.cpp:128 >> +} > > Repeating very similar implementations is not good. > I think it's ok to use a preprocessor macro to generate functions. Good point. I realized that there are unfortunate ad-hoc naming trend and ifdefs that prevent us from generating whole function. So I extracted error check part instead.

Comment on attachment 123283 [details] Patch Attachment 123283 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11297076

Comment on attachment 123283 [details] Patch Attachment 123283 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11313079

Comment on attachment 123283 [details] Patch Pleas fix build.

Created attachment 123663 [details] Attemt to fix broken builds

Comment on attachment 123663 [details] Attemt to fix broken builds Attachment 123663 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11335299 New failing tests: fast/dom/window-inner-size-scaling.html fast/frames/frame-set-rotation-hit.html fast/dom/iframe-inner-size-scaling.html

Created attachment 123760 [details] Updated to ToT for catching up new tests

Comment on attachment 123760 [details] Updated to ToT for catching up new tests View in context: https://bugs.webkit.org/attachment.cgi?id=123760&action=review > Source/WebCore/testing/InternalSettings.cpp:51 > +#define INTERNAL_SETTINGS_GUARD_FOR_SETTINGS_RETURN(returnValue) \ http://www.webkit.org/coding/coding-style.html#names-define-non-const

Created attachment 123954 [details] Patch for landing

Kent-san, thank you for taking a look! > > Source/WebCore/testing/InternalSettings.cpp:51 > > +#define INTERNAL_SETTINGS_GUARD_FOR_SETTINGS_RETURN(returnValue) \ > > http://www.webkit.org/coding/coding-style.html#names-define-non-const Fixed in the landing patch.

Comment on attachment 123954 [details] Patch for landing Rejecting attachment 123954 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11344621

Landed at http://trac.webkit.org/changeset/105900

The addition of V8InternalsSettings.* to WebCore.gyp is causing unnecessary rebuilds on Windows because it doesn't seem to exist. Will that file exist soon, or is it not supposed to be there?

Oh, it looks like it's supposed to be V8InternalSettings.h, not V8Internal_s_Settings.h. Could you fix that?

Please don't use ::-webkit-scrollbar style to hide scrollbars. This technique doesn't work in WebKit1 windows, so affects all the Mac DRT results.

(In reply to comment #20) > Please don't use ::-webkit-scrollbar style to hide scrollbars. This technique doesn't work in WebKit1 windows, so affects all the Mac DRT results. @smfr looks like you are talking about about a different bug.

Sorry, I just looked at the last change to: * compositing/geometry/fixed-position-composited-page-scale-down.html: * compositing/geometry/fixed-position-composited-page-scale.html: * compositing/geometry/fixed-position-composited-switch.html: * compositing/geometry/fixed-position-iframe-composited-page-scale-down.html: * compositing/geometry/fixed-position-iframe-composited-page-scale.html: * compositing/geometry/fixed-position-transform-composited-page-scale-down.html: * compositing/geometry/fixed-position-transform-composited-page-scale.html: I really wanted bug 71225.