|
|
Created:
4 years, 5 months ago by ghost stip (do not use) Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlso use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation.
BUG=622858
Committed: https://crrev.com/838630d2efaf6ad90ab5b3c18bd752ac9d4fd144
Cr-Commit-Position: refs/heads/master@{#402358}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change SubframeRetainsZoomOnNavigation as well. #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom. BUG=622858 ========== to ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom. BUG=622858 ==========
stip@chromium.org changed reviewers: + creis@chromium.org, wjmaclean@chromium.org
ptal! This fixes up one test which got missed in https://codereview.chromium.org/2097063002.
On 2016/06/27 21:05:41, stip wrote: > ptal! > > This fixes up one test which got missed in > https://codereview.chromium.org/2097063002. It wasn't so much missed ... I figured this test was passing ok on the bots so I'd leave it as was. But I'm ok with the change.
If we're trying to avoid future flakiness, it looks like there might be one more to convert. https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... content/browser/iframe_zoom_browsertest.cc:422: EXPECT_DOUBLE_EQ( What about this one?
https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... content/browser/iframe_zoom_browsertest.cc:422: EXPECT_DOUBLE_EQ( On 2016/06/27 21:23:08, Charlie Reis wrote: > What about this one? Sure, given the use of log/exp in converting between zoom levels and zoom factors, and given a 1% change in zoom is close enough for testing, then it seems reasonable to change here as well.
Description was changed from ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom. BUG=622858 ========== to ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation. BUG=622858 ==========
AllFramesGetDefaultZoom is still failing on https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%...: https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%... (I used the android_n5x_swarming_rel bot to check). Either way I updated SubframeRetainsZoomOnNavigation as well. https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... content/browser/iframe_zoom_browsertest.cc:422: EXPECT_DOUBLE_EQ( On 2016/06/27 21:53:58, wjmaclean wrote: > On 2016/06/27 21:23:08, Charlie Reis wrote: > > What about this one? > > Sure, given the use of log/exp in converting between zoom levels and zoom > factors, and given a 1% change in zoom is close enough for testing, then it > seems reasonable to change here as well. Done.
On 2016/06/27 23:21:16, stip wrote: > AllFramesGetDefaultZoom is still failing on > https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%...: > https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%... > > (I used the android_n5x_swarming_rel bot to check). Either way I updated > SubframeRetainsZoomOnNavigation as well. > > https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... > File content/browser/iframe_zoom_browsertest.cc (right): > > https://codereview.chromium.org/2100233002/diff/1/content/browser/iframe_zoom... > content/browser/iframe_zoom_browsertest.cc:422: EXPECT_DOUBLE_EQ( > On 2016/06/27 21:53:58, wjmaclean wrote: > > On 2016/06/27 21:23:08, Charlie Reis wrote: > > > What about this one? > > > > Sure, given the use of log/exp in converting between zoom levels and zoom > > factors, and given a 1% change in zoom is close enough for testing, then it > > seems reasonable to change here as well. > > Done. LGTM
The CQ bit was checked by stip@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
Message was sent while issue was closed.
Description was changed from ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation. BUG=622858 ========== to ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation. BUG=622858 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation. BUG=622858 ========== to ========== Also use EXPECT_NEAR in AllFramesGetDefaultZoom and SubframeRetainsZoomOnNavigation. BUG=622858 Committed: https://crrev.com/838630d2efaf6ad90ab5b3c18bd752ac9d4fd144 Cr-Commit-Position: refs/heads/master@{#402358} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/838630d2efaf6ad90ab5b3c18bd752ac9d4fd144 Cr-Commit-Position: refs/heads/master@{#402358} |