Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265213002:
Fix device scale factor for interstitials. (Closed)
|
Created:
4 years, 4 months ago by wjmaclean Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix device scale factor for interstitials.
Implement GetScreenInfo() and UpdateDeviceScaleFactor() for
InterstitialPageImpl so that interstitial pages get the correct device
scale factor both when they're initially created, and also if the window
they're in gets dragged between displays with different device scale
factors.
BUG=637462
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/26840c53c281e98b2f33a5e94aa7f04d372eb9c6
Cr-Commit-Position: refs/heads/master@{#414189}
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 1
Patch Set 3 : Handle missing WebContents for Mac. #
Total comments: 4
Patch Set 4 : Address comments. #
Messages
Total messages: 32 (20 generated)
Description was changed from ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 ========== to ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Initial CL w/o tests just to run on the trybots.
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wjmaclean@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
wjmaclean@chromium.org changed reviewers: + avi@chromium.org
Avi, could you please take a look? I'm still investigating the Mac failures (seems the InterstitialPage on Mac doesn't always have a WebContentsImpl?). It seems to Run fine on my Mac workstation (but it's 10.11.6 - El Capitan ... the failures are happening on 10.9, and seem to be somewhat flakey?). I'm not that familiar with why these would be different, can you suggest anyone who might know?
https://codereview.chromium.org/2265213002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2265213002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:116: I'm aware of this blank line, and will remove it in the next patch.
On 2016/08/23 18:00:56, wjmaclean (OOO Aug 13 - 21) wrote: > Avi, could you please take a look? > > I'm still investigating the Mac failures (seems the InterstitialPage on Mac > doesn't always have a WebContentsImpl?). It seems to Run fine on my Mac > workstation (but it's 10.11.6 - El Capitan ... the failures are happening on > 10.9, and seem to be somewhat flakey?). I'm not that familiar with why these > would be different, can you suggest anyone who might know? Offhand, no idea, sorry.
lgtm
The CQ bit was checked by wjmaclean@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.
wjmaclean@chromium.org changed reviewers: + thestig@chromium.org
thestig@ - Would you please look at chrome_site_per_process_browsertest? avi@ - minor change to handle cases where web_contents_ is null, still l-g-t-m?
s lgtm
lgtm https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:59: net::EmbeddedTestServer https_server_expired_; It would be nice if this wasn't public and there was a GetURL() method instead, but if you want to land this as is and polish it later, that's fine by me. https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:98: browser()->tab_strip_model()->GetActiveWebContents()); You can also save the GetActiveWebContents() return pointer and use it below so you don't have to write code like this.
Done, thanks for the feedback! https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:59: net::EmbeddedTestServer https_server_expired_; On 2016/08/24 18:33:51, Lei Zhang wrote: > It would be nice if this wasn't public and there was a GetURL() method instead, > but if you want to land this as is and polish it later, that's fine by me. Done. I've added a function expired_cert_test_server() to match the embedded_test_server() function already in use in the test, since that gives better consistency as to how the test servers are accessed. Hope that's ok, but if not we can change it later. https://codereview.chromium.org/2265213002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:98: browser()->tab_strip_model()->GetActiveWebContents()); On 2016/08/24 18:33:50, Lei Zhang wrote: > You can also save the GetActiveWebContents() return pointer and use it below so > you don't have to > write > code > like > this. Done.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2265213002/#ps60001 (title: "Address comments.")
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.
Description was changed from ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix device scale factor for interstitials. Implement GetScreenInfo() and UpdateDeviceScaleFactor() for InterstitialPageImpl so that interstitial pages get the correct device scale factor both when they're initially created, and also if the window they're in gets dragged between displays with different device scale factors. BUG=637462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/26840c53c281e98b2f33a5e94aa7f04d372eb9c6 Cr-Commit-Position: refs/heads/master@{#414189} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/26840c53c281e98b2f33a5e94aa7f04d372eb9c6 Cr-Commit-Position: refs/heads/master@{#414189} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
