Use consistent background color for WKWebViews. (issue 1682273002 by kkhorimoto@chromium.org)

[kkhor...@chromium.org]

Reviewers: stuartmorgan, gchatz
CL: https://codereview.chromium.org/1682273002/

Message:
I noticed this while investigating scroll fixes.  The main web views for the app
used white with 0.2 brightness as the WKWebView's backgroundColor, while
interstitials used the color from UIWebView's scroll view's background.

Description:
Use consistent background color for WKWebViews.

UIWebViews have their own default background color, while WKWebViews
will interpolate between white and the WKWebView's |-backgroundColor|
while the content is being zoomed.  An earlier solution to this problem
was to use UIWebView's background color behind web views, but this CL
consolidates the backgroundColors for all WKWebViews.

BUG=none

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+5, -16 lines):
  M ios/web/web_state/ui/crw_web_controller.mm
  M ios/web/web_state/ui/crw_web_view_content_view.mm
  M ios/web/web_state/web_view_internal_creation_util.mm


Index: ios/web/web_state/ui/crw_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index 8a2ccccdd8ee8ad44f18d52533f7bc8c339892f3..c63b45df123bc4c0c223f7a8052f8a9be32bc298 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -1132,7 +1132,6 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   [webView setTag:kWebViewTag];
   [webView setAutoresizingMask:UIViewAutoresizingFlexibleWidth |
                                UIViewAutoresizingFlexibleHeight];
-  [webView setBackgroundColor:[UIColor colorWithWhite:0.2 alpha:1.0]];
 
   // Create a dependency between the |webView| pan gesture and BVC side swipe
   // gestures. Note: This needs to be added before the longPress recognizers
Index: ios/web/web_state/ui/crw_web_view_content_view.mm
diff --git a/ios/web/web_state/ui/crw_web_view_content_view.mm b/ios/web/web_state/ui/crw_web_view_content_view.mm
index 36ff49cbc3c78ce80e4d4072c27ab8bff02b4642..a583215e0a0c83f3c84a3ecebd0374efefcd02b8 100644
--- a/ios/web/web_state/ui/crw_web_view_content_view.mm
+++ b/ios/web/web_state/ui/crw_web_view_content_view.mm
@@ -9,17 +9,6 @@
 #include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
 
-namespace {
-
-// Background color RGB values for the content view which is displayed when the
-// |_webView| is offset from the screen due to user interaction. Displaying this
-// background color is handled by UIWebView but not WKWebView, so it needs to be
-// set in CRWWebViewContentView to support both. The color value matches that
-// used by UIWebView.
-const CGFloat kBackgroundRGBComponents[] = {0.75f, 0.74f, 0.76f};
-
-}  // namespace
-
 @interface CRWWebViewContentView () {
   // The web view being shown.
   base::scoped_nsobject<UIView> _webView;
@@ -75,10 +64,6 @@ const CGFloat kBackgroundRGBComponents[] = {0.75f, 0.74f, 0.76f};
     self.autoresizingMask =
         UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
     [self addSubview:_webView];
-    self.backgroundColor = [UIColor colorWithRed:kBackgroundRGBComponents[0]
-                                           green:kBackgroundRGBComponents[1]
-                                            blue:kBackgroundRGBComponents[2]
-                                           alpha:1.0];
     // The frame needs to be set immediately after the web view is added
     // as a subview. The change in the frame triggers drawing operations and
     // if not done after it's added as a subview, the web view exhibits
Index: ios/web/web_state/web_view_internal_creation_util.mm
diff --git a/ios/web/web_state/web_view_internal_creation_util.mm b/ios/web/web_state/web_view_internal_creation_util.mm
index cbf01d6474f40f6926eea40971cc5fdf629c8eb4..2ac42ed59d8e0c0fde32f2daa4784b95bd783205 100644
--- a/ios/web/web_state/web_view_internal_creation_util.mm
+++ b/ios/web/web_state/web_view_internal_creation_util.mm
@@ -41,6 +41,9 @@ web::WeakNSObjectCounter& GetActiveWKWebViewCounter() {
   return active_wk_web_view_counter;
 }
 
+// The brightness of the web views' background colors.
+const CGFloat kWebViewBackgroundColorBrightness = 0.2;
+
 // Decides if web views can be created.
 bool gAllowWebViewCreation = NO;
 
@@ -184,6 +187,8 @@ WKWebView* CreateWKWebView(CGRect frame,
 #endif
   WKWebView* result =
       [[WKWebView alloc] initWithFrame:frame configuration:configuration];
+  result.backgroundColor =
+      [UIColor colorWithWhite:kWebViewBackgroundColorBrightness alpha:1.0];
 #if !defined(NDEBUG)
   gAllowWebViewCreation = previous_allow_web_view_creation_value;
 #endif

[gch...@chromium.org]

On 2016/02/09 23:50:27, kkhorimoto_ wrote:
> I noticed this while investigating scroll fixes.  The main web views for the
app
> used white with 0.2 brightness as the WKWebView's backgroundColor, while
> interstitials used the color from UIWebView's scroll view's background.

Would this background show up when a transientContentView is visible (and thus
the WebViewContentView is not)?

https://codereview.chromium.org/1682273002/

[kkhor...@chromium.org]

On 2016/02/10 01:55:26, gchatz wrote:
> On 2016/02/09 23:50:27, kkhorimoto_ wrote:
> > I noticed this while investigating scroll fixes.  The main web views for the
> app
> > used white with 0.2 brightness as the WKWebView's backgroundColor, while
> > interstitials used the color from UIWebView's scroll view's background.
> 
> Would this background show up when a transientContentView is visible (and thus
> the WebViewContentView is not)?

If you're referring to interstitials, then yes (they are backed by WKWebViews
and displayed via CRWWebViewContentViews).  It's possible that other native
content views would have a transparent background, but that's by design.

https://codereview.chromium.org/1682273002/

[kkhor...@chromium.org]

On 2016/02/10 02:18:40, kkhorimoto_ wrote:
> On 2016/02/10 01:55:26, gchatz wrote:
> > On 2016/02/09 23:50:27, kkhorimoto_ wrote:
> > > I noticed this while investigating scroll fixes.  The main web views for
the
> > app
> > > used white with 0.2 brightness as the WKWebView's backgroundColor, while
> > > interstitials used the color from UIWebView's scroll view's background.
> > 
> > Would this background show up when a transientContentView is visible (and
thus
> > the WebViewContentView is not)?
> 
> If you're referring to interstitials, then yes (they are backed by WKWebViews
> and displayed via CRWWebViewContentViews).  It's possible that other native
> content views would have a transparent background, but that's by design.

I also verified that the bug you fixed when introducing the background color no
longer repros.

https://codereview.chromium.org/1682273002/

[gch...@chromium.org]

On 2016/02/10 02:18:40, kkhorimoto_ wrote:
> On 2016/02/10 01:55:26, gchatz wrote:
> > On 2016/02/09 23:50:27, kkhorimoto_ wrote:
> > > I noticed this while investigating scroll fixes.  The main web views for
the
> > app
> > > used white with 0.2 brightness as the WKWebView's backgroundColor, while
> > > interstitials used the color from UIWebView's scroll view's background.
> > 
> > Would this background show up when a transientContentView is visible (and
thus
> > the WebViewContentView is not)?
> 
> If you're referring to interstitials, then yes (they are backed by WKWebViews
> and displayed via CRWWebViewContentViews).  It's possible that other native
> content views would have a transparent background, but that's by design.

thanks! lgtm

https://codereview.chromium.org/1682273002/

[stuart...@chromium.org]

https://codereview.chromium.org/1682273002/diff/20001/ios/web/web_state/ui/crw_web_view_content_view.mm
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682273002/diff/20001/ios/web/web_state/ui/crw_web_view_content_view.mm#oldcode78
ios/web/web_state/ui/crw_web_view_content_view.mm:78:
self.backgroundColor = [UIColor colorWithRed:kBackgroundRGBComponents[0]
Why don't we still need a background color on the content view; are
there not cases where the web view draws what's behind it, rather than
itself?

https://codereview.chromium.org/1682273002/

[kkhor...@chromium.org]

https://codereview.chromium.org/1682273002/diff/20001/ios/web/web_state/ui/crw_web_view_content_view.mm
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682273002/diff/20001/ios/web/web_state/ui/crw_web_view_content_view.mm#oldcode78
ios/web/web_state/ui/crw_web_view_content_view.mm:78

:
self.backgroundColor = [UIColor colorWithRed:kBackgroundRGBComponents[0]
On 2016/02/11 17:04:34, stuartmorgan wrote:
> Why don't we still need a background color on the content view; are
there not
> cases where the web view draws what's behind it, rather than itself?

I'm not sure what you mean about the web view drawing what's behind it
rather than itself.  AFAICT, all web rendering occurs in WKContentView,
which is one of the subviews of WKWebView's scroll view.  So if a page
is attempting to render a semi-transparent element, the subview in
WKContentView would be transparent, but the WKWebView's background color
will remain constant in the background.  After inspecting WKWebView's
source, it doesn't look like the background color is ever updated
outside of the public UIView API.  It's only the scroll view whose
backgroundColor changes, but since that's a subview of the WKWebView, it
should never show what's behind.

https://codereview.chromium.org/1682273002/

[stuart...@chromium.org]

Makes sense; lgtm

https://codereview.chromium.org/1682273002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1682273002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1682273002/20001


https://codereview.chromium.org/1682273002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chromium_gn_compile_dbg/builds/20863)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/20892)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/20789)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/114877)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/92218)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/92196)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/92201)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/145232)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/115037)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/124105)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/165492)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_clobber_rel_ng/builds/129286)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/157692)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/142292)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/179935)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/129803)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ninja/builds/171394)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/158186)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/65354)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/178960)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/102360)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/172747)

https://codereview.chromium.org/1682273002/