New issue
Advanced search Search tips

Issue 696895 link

Starred by 1 user

Issue metadata

Status: Archived
Owner: ----
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Make WebViewClient/WebWidgetClient parameters non-null and remove redundant null checks.

Project Member Reported by slangley@chromium.org, Feb 28 2017

Issue description

Currently, it is legal to pass nullptr as WebViewClient/WebWidgetClient when creating WebView/WebWidget, but this makes the code hard to understand.
 
Components: -Blink>Internals Blink>Internals>Frames
Due to the object model it's probably easier to add an ASSERT() in ::create() methods.

e.g.

WebViewClient() has a protected dtor so we cannot take ownership in WebView.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5

commit 9df2c54436a76ebaf6bfcc317815e776e4a0a7f5
Author: slangley <slangley@chromium.org>
Date: Tue Mar 28 23:20:08 2017

Ensure that the WebWidgetClient cannot be a nullptr by adding DCHECK()s.

There was only one call site in print_web_view_helper where a nullptr was being passed instead of a valid WebWidgetClient, to fix this the following changes were also made:

* Make WebWidgetClient dtor public so the class can be instantiated.
* Pass a valid WebWidgetClient in the print_web_view_helper

A subsequent CL will analyze and remove m_client null checks in WebFrameWidgetImpl.

I left WebFrameViewClient as a ref as this appears to be internal to blink and will adhere to blink standards.

BUG= 696895 

Review-Url: https://codereview.chromium.org/2773003002
Cr-Commit-Position: refs/heads/master@{#460235}

[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/content/shell/test_runner/web_view_test_client.h
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/content/shell/test_runner/web_widget_test_client.h
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/third_party/WebKit/Source/web/WebViewFrameWidget.cpp
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/third_party/WebKit/Source/web/WebViewFrameWidget.h
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/third_party/WebKit/public/web/WebViewClient.h
[modify] https://crrev.com/9df2c54436a76ebaf6bfcc317815e776e4a0a7f5/third_party/WebKit/public/web/WebWidgetClient.h

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a6375e628f5405f33f9061ee25644eaa90a81c2

commit 1a6375e628f5405f33f9061ee25644eaa90a81c2
Author: slangley <slangley@chromium.org>
Date: Wed Mar 29 06:09:25 2017

Add canUpdateLayout & canHandleGestureEvent to WebViewClient.

This work is in preperation for work to make it illegal to pass a nullptr
for WebViewClient when instantiting a WebView object.

There are a handful of places in WebViewImpl where the logic changes based
on the nullness of WebViewClient. These changes will preserve this logic when
a non-null WebViewClient is supplied.

This include:
- Returning WebInputEventResult::NotHandled from handleGestureEvent if the
  client specifically opts out of handling the event.
- Not processing layout updates if the client opts out.

After removing null checks in WebViewImpl the requirement for these new methods
are to be reviewed and they should be removed if possible.

Also, in preparation for forcing clients to pass a non null WebViewClient
I made the dtor for WebViewClient public so it can be instantiated without
derivation.

BUG= 696895 

Review-Url: https://codereview.chromium.org/2762553002
Cr-Commit-Position: refs/heads/master@{#460298}

[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/components/plugins/renderer/webview_plugin.h
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/content/renderer/render_view_impl.cc
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/content/renderer/render_view_impl.h
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/content/shell/test_runner/web_view_test_client.cc
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/content/shell/test_runner/web_view_test_client.h
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/third_party/WebKit/Source/web/tests/FrameTestHelpers.h
[modify] https://crrev.com/1a6375e628f5405f33f9061ee25644eaa90a81c2/third_party/WebKit/public/web/WebViewClient.h

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/943648dbbf5a074d645d71c8108c0caedff7967a

commit 943648dbbf5a074d645d71c8108c0caedff7967a
Author: slangley <slangley@chromium.org>
Date: Thu Mar 30 01:09:10 2017

Audit use of m_client in WebFrameWidgetImpl and remove null checks.

Now that a valid WebWidgetClient must always be passed when creating a
WebFrameWidget we can remove some of the redundant m_client == nullptr checks.

m_client can now become nullptr if close() is called on the WebFrameWidget,
but this is generally caught by checking m_layerTreeView.

To be safe if(m_client) checks have been replaced with DCHECK(m_client).

BUG= 696895 

Review-Url: https://codereview.chromium.org/2775113002
Cr-Commit-Position: refs/heads/master@{#460612}

[modify] https://crrev.com/943648dbbf5a074d645d71c8108c0caedff7967a/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Owner: ----
Status: Archived (was: Started)

Sign in to add a comment