Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721883003:
Put Top Control resizing change behind a flag. (Closed)
|
Created:
4 years, 10 months ago by bokan Modified:
4 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, blink-reviews, dglazkov+blink, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPut Top Control resizing change behind a flag.
The change to how top controls affect VH units and the initial containing
block was made in r370425. We've decided to hold off on shipping that change
for now so I've placed this change behind a flag. Unless the inert-top-controls
experiment is enabled, the behavior is unchanged from M49.
BUG=428132
Committed: https://crrev.com/cc87033da754d1ec0a34e0dd39798f67fc132085
Cr-Commit-Position: refs/heads/master@{#377309}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Update tests to use the flag #Patch Set 4 : Added flag to histograms.xml #Patch Set 5 : Use RuntimeEnabledFeatures #Patch Set 6 : ...and update tests to use the flag #
Messages
Total messages: 46 (20 generated)
bokan@chromium.org changed reviewers: + avi@chromium.org, kenrb@chromium.org, thakis@chromium.org
+kenrb@ for common_param_traits_macros +thakis@ for about_flags.cc, generated_resources.grd and Blink +avi@ for content/public, content/browser, and content/renderer
ipc lgtm
Someone who's familiar with this feature should look at FrameView.cpp and WebViewImpl.cpp. The rest lgtm.
bokan@chromium.org changed reviewers: + ymalik@chromium.org
+ymalik@ to double-check WebViewImpl/FrameView changes
WebViewImpl/FrameView changes lgtm
On 2016/02/23 15:24:50, ymalik1 wrote: > WebViewImpl/FrameView changes lgtm Is this flag temporary and going away soon?
On 2016/02/23 15:41:39, Avi wrote: > On 2016/02/23 15:24:50, ymalik1 wrote: > > WebViewImpl/FrameView changes lgtm > > Is this flag temporary and going away soon?
On 2016/02/23 15:42:07, bokan wrote: > On 2016/02/23 15:41:39, Avi wrote: > > On 2016/02/23 15:24:50, ymalik1 wrote: > > > WebViewImpl/FrameView changes lgtm > > > > Is this flag temporary and going away soon? Yes, hopefully within 2-3 milestones.
lgtm
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, avi@chromium.org, thakis@chromium.org, ymalik@chromium.org Link to the patchset: https://codereview.chromium.org/1721883003/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, avi@chromium.org, thakis@chromium.org, ymalik@chromium.org Link to the patchset: https://codereview.chromium.org/1721883003/#ps40001 (title: "Update tests to use the flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 ========== to ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 ==========
bokan@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ for histograms.xml
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/60001
For new things, it's better to use the feature api - see go/feature-api-announcement. Although there are existing --enable/--disable flags, the goal (as discussed with chrome eng review) is to transition these to Feature API, so it would be better to use that here rather than having to migrate this later. It should be pretty straight forward to change this, see the go link above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/80001
The CQ bit was unchecked by bokan@chromium.org
On 2016/02/24 00:50:19, Alexei Svitkine (slow) wrote: > For new things, it's better to use the feature api - see > go/feature-api-announcement. Although there are existing --enable/--disable > flags, the goal (as discussed with chrome eng review) is to transition these to > Feature API, so it would be better to use that here rather than having to > migrate this later. It should be pretty straight forward to change this, see the > go link above. Actually, that reminded me this is more appropriate as a Blink RuntimeEnabledFeature. Updated.
thakis@: Is the updated version still l-g-t-m?
yes, lgtm
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, avi@chromium.org, ymalik@chromium.org Link to the patchset: https://codereview.chromium.org/1721883003/#ps100001 (title: "...and update tests to use the flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883003/100001
Message was sent while issue was closed.
Description was changed from ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 ========== to ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 ========== to ========== Put Top Control resizing change behind a flag. The change to how top controls affect VH units and the initial containing block was made in r370425. We've decided to hold off on shipping that change for now so I've placed this change behind a flag. Unless the inert-top-controls experiment is enabled, the behavior is unchanged from M49. BUG=428132 Committed: https://crrev.com/cc87033da754d1ec0a34e0dd39798f67fc132085 Cr-Commit-Position: refs/heads/master@{#377309} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cc87033da754d1ec0a34e0dd39798f67fc132085 Cr-Commit-Position: refs/heads/master@{#377309} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
