Change all test cases to use WebViewBase instead of WebViewImpl.

Change all test cases to use WebViewBase instead of WebViewImpl.

This CL changes all of the test cases to use the abstract WebViewBase class in
place of WebViewImpl. This should be the final CL to decouple the use of 
WebViewImpl in web/ and move everything over to WebViewBase.

As before, there were a number of methods defined in WebViewImpl that needed to
be made virtual in WebViewBase and then overridden in WebViewImpl.

Also, it was not possible to use CORE_EXPORT or MODULES_EXPORT to export the
statically defined symbols from WebViewBase, so I used PLATFORM_EXPORT instead.

A number of symbols were being transitively included, so in places needed to
add include files to bring the symbols into scope.

BUG=712963
Review-Url: https://codereview.chromium.org/2860673002
Cr-Commit-Position: refs/heads/master@{#469575}
Committed: https://chromium.googlesource.com/chromium/src/+/ba9abc4c70ae99cf7fc03adbd48258cea2514595

[slangley, 2017-05-03 03:58:00]

Description was changed from

==========
Change all test cases to use WebViewBase instead of WebViewImpl.

BUG=
==========

to

==========
Change all test cases to use WebViewBase instead of WebViewImpl.

This CL changes all of the test cases to use the abstract WebViewBase class in
place of WebViewImpl. This should be the final CL to decouple the use of 
WebViewImpl in web/ and move everything over to WebViewBase.

As before, there were a number of methods defined in WebViewImpl that needed to
be made virtual in WebViewBase and then overridden in WebViewImpl.

Also, it was not possible to use CORE_EXPORT or MODULES_EXPORT to export the
statically defined symbols from WebViewBase, so I used PLATFORM_EXPORT instead.

A number of symbols were being transitively included, so in places needed to
add include files to bring the symbols into scope.

BUG=712963
==========

[slangley, 2017-05-03 03:58:22]

slangley@chromium.org changed reviewers:
+ sashab@chromium.org

[sashab, 2017-05-03 04:05:22]

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT static
WebViewBase* Create(WebViewClient*,
I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code isn't in
platform/. Check with someone else though

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:190: virtual
BrowserControls& GetBrowserControls() = 0;
Make them private? And add the test as a friend class

[sashab, 2017-05-03 04:34:19]

Other than that LGTM

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp:48:
WebViewBase::SetCurrentInputEventForTest(event);
Maybe next time do this in a separate patch so its easier to see

[slangley, 2017-05-03 21:43:07]

slangley@chromium.org changed reviewers:
+ haraken@chromium.org

[slangley, 2017-05-03 21:43:07]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-05-03 21:43:08]

haraken@ - ptal. There are a couple of nits, WDYT?

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT static
WebViewBase* Create(WebViewClient*,
On 2017/05/03 at 04:05:22, sashab wrote:
> I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code isn't in
platform/. Check with someone else though

I think because of the layering, PLATFORM_EXPORT makes more sense (as it's in
platform rather than web/) but i don't feel strongly either way. I'll see what
haraken@ thinks.

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:190: virtual
BrowserControls& GetBrowserControls() = 0;
On 2017/05/03 at 04:05:22, sashab wrote:
> Make them private? And add the test as a friend class

In general for these changes my idea is:

a) Move the code to it's new home so we can decouple classes in web/.
b) Come back and refactor where it makes sense. I plan to do this as part of
cleaning up the Client code that exists mainly to allow calls from core -> web
that will become redundant with these changes. In general I am trying not to
introduce any logic changes (where possible) while shuffling the code.

[slangley, 2017-05-03 21:43:12]

The CQ bit was unchecked by slangley@chromium.org

[slangley, 2017-05-03 21:43:26]

The CQ bit was checked by slangley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 21:44:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 23:18:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 23:18:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[haraken, 2017-05-04 05:30:42]

LGTM with a couple of questions.

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT static
WebViewBase* Create(WebViewClient*,
On 2017/05/03 21:43:08, slangley wrote:
> On 2017/05/03 at 04:05:22, sashab wrote:
> > I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code isn't
in
> platform/. Check with someone else though
> 
> I think because of the layering, PLATFORM_EXPORT makes more sense (as it's in
> platform rather than web/) but i don't feel strongly either way. I'll see what
> haraken@ thinks.

This file is in core/, not platform/. Why can't we use CORE_EXPORT?

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:224: PLATFORM_EXPORT
static void SetCurrentInputEventForTest(const WebInputEvent*);

Ditto.

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2167: void
WebViewBase::SetCurrentInputEventForTest(const WebInputEvent* event) {

It looks a bit strange that WebViewBase's method is defined in
WebViewImpl.cpp... ?

[slangley, 2017-05-04 09:15:34]

Thanks!

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT static
WebViewBase* Create(WebViewClient*,
On 2017/05/04 at 05:30:42, haraken wrote:
> On 2017/05/03 21:43:08, slangley wrote:
> > On 2017/05/03 at 04:05:22, sashab wrote:
> > > I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code isn't
in
> > platform/. Check with someone else though
> > 
> > I think because of the layering, PLATFORM_EXPORT makes more sense (as it's
in
> > platform rather than web/) but i don't feel strongly either way. I'll see
what
> > haraken@ thinks.
> 
> This file is in core/, not platform/. Why can't we use CORE_EXPORT?

CORE_EXPORT has an additional compiler defined constant required to actually
export (BLINK_CORE_IMPLEMENTATION) - in some cases this is not defined and hence
the symbols to not end up being exported. BLINK_EXPORT and PLATFORM_EXPORT do
not have this issue. Once we finish the move we can remove the exporting of
these symbols.

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2167: void
WebViewBase::SetCurrentInputEventForTest(const WebInputEvent* event) {
On 2017/05/04 at 05:30:42, haraken wrote:
> It looks a bit strange that WebViewBase's method is defined in
WebViewImpl.cpp... ?

This is the same as WebView::Create(), the other option would be to move the
static to WebViewBase, but it didn't seem worth it.

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp:48:
WebViewBase::SetCurrentInputEventForTest(event);
On 2017/05/03 at 04:34:18, sashab wrote:
> Maybe next time do this in a separate patch so its easier to see

Will keep it in mind.

[haraken, 2017-05-04 15:14:32]

(Tokyo is a vacation week, and I'll be slow at reviewing.)

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT static
WebViewBase* Create(WebViewClient*,
On 2017/05/04 09:15:34, slangley wrote:
> On 2017/05/04 at 05:30:42, haraken wrote:
> > On 2017/05/03 21:43:08, slangley wrote:
> > > On 2017/05/03 at 04:05:22, sashab wrote:
> > > > I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code
isn't
> in
> > > platform/. Check with someone else though
> > > 
> > > I think because of the layering, PLATFORM_EXPORT makes more sense (as it's
> in
> > > platform rather than web/) but i don't feel strongly either way. I'll see
> what
> > > haraken@ thinks.
> > 
> > This file is in core/, not platform/. Why can't we use CORE_EXPORT?
> 
> CORE_EXPORT has an additional compiler defined constant required to actually
> export (BLINK_CORE_IMPLEMENTATION) - in some cases this is not defined and
hence
> the symbols to not end up being exported. BLINK_EXPORT and PLATFORM_EXPORT do
> not have this issue. Once we finish the move we can remove the exporting of
> these symbols.

Hmm. Maybe I'm a bit behind.

No one in core/ is using BLINK_EXPORT. They are using CORE_EXPORT and working.
Why can WebViewBase not follow the pattern?

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2167: void
WebViewBase::SetCurrentInputEventForTest(const WebInputEvent* event) {
On 2017/05/04 09:15:34, slangley wrote:
> On 2017/05/04 at 05:30:42, haraken wrote:
> > It looks a bit strange that WebViewBase's method is defined in
> WebViewImpl.cpp... ?
> 
> This is the same as WebView::Create(), the other option would be to move the
> static to WebViewBase, but it didn't seem worth it.

Can we make current_input_event_ a non-static member and use a virtual method?
i.e., WebViewBase::SetCurrentInputEventForTest redirects to
WebViewImpl::SetCurrentInputEventForTest.

If it really needs to be a static variable, I don't think WebViewImpl is a right
place to put it.

[slangley, 2017-05-05 00:27:33]

On 2017/05/04 at 15:14:32, haraken wrote:
> (Tokyo is a vacation week, and I'll be slow at reviewing.)
> 
>
https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/exported/WebViewBase.h (right):
> 
>
https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/exported/WebViewBase.h:69: PLATFORM_EXPORT
static WebViewBase* Create(WebViewClient*,
> On 2017/05/04 09:15:34, slangley wrote:
> > On 2017/05/04 at 05:30:42, haraken wrote:
> > > On 2017/05/03 21:43:08, slangley wrote:
> > > > On 2017/05/03 at 04:05:22, sashab wrote:
> > > > > I think BLINK_EXPORT is better than PLATFORM_EXPORT, since this code
isn't
> > in
> > > > platform/. Check with someone else though
> > > > 
> > > > I think because of the layering, PLATFORM_EXPORT makes more sense (as
it's
> > in
> > > > platform rather than web/) but i don't feel strongly either way. I'll
see
> > what
> > > > haraken@ thinks.
> > > 
> > > This file is in core/, not platform/. Why can't we use CORE_EXPORT?
> > 
> > CORE_EXPORT has an additional compiler defined constant required to actually
> > export (BLINK_CORE_IMPLEMENTATION) - in some cases this is not defined and
hence
> > the symbols to not end up being exported. BLINK_EXPORT and PLATFORM_EXPORT
do
> > not have this issue. Once we finish the move we can remove the exporting of
> > these symbols.
> 
> Hmm. Maybe I'm a bit behind.
> 
> No one in core/ is using BLINK_EXPORT. They are using CORE_EXPORT and working.
Why can WebViewBase not follow the pattern?

I removed the static methods that needed this, so problem solved :)
 
> 
>
https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2860673002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2167: void
WebViewBase::SetCurrentInputEventForTest(const WebInputEvent* event) {
> On 2017/05/04 09:15:34, slangley wrote:
> > On 2017/05/04 at 05:30:42, haraken wrote:
> > > It looks a bit strange that WebViewBase's method is defined in
> > WebViewImpl.cpp... ?
> > 
> > This is the same as WebView::Create(), the other option would be to move the
> > static to WebViewBase, but it didn't seem worth it.
> 
> Can we make current_input_event_ a non-static member and use a virtual method?
i.e., WebViewBase::SetCurrentInputEventForTest redirects to
WebViewImpl::SetCurrentInputEventForTest.
> 
> If it really needs to be a static variable, I don't think WebViewImpl is a
right place to put it.

Turns out this was only used in one test case that had a reference to web_view_
available, so was able to move this to an instance method.

Thanks!

[slangley, 2017-05-05 00:27:58]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-05-05 00:27:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2860673002/#ps20001
(title: "Address code review comments.")

[commit-bot: I haz the power, 2017-05-05 00:28:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-05-05 00:54:00]

LGTM

[haraken, 2017-05-05 00:54:38]

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/exported/WebViewBase.h:70: //                    
                       WebPageVisibilityState);

Remove this.

[commit-bot: I haz the power, 2017-05-05 02:27:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493944078316150,
"parent_rev": "a7f495e07e33659fed8591fcabf5f3616c1042a2", "commit_rev":
"ba9abc4c70ae99cf7fc03adbd48258cea2514595"}

[commit-bot: I haz the power, 2017-05-05 02:27:48]

Description was changed from

==========
Change all test cases to use WebViewBase instead of WebViewImpl.

This CL changes all of the test cases to use the abstract WebViewBase class in
place of WebViewImpl. This should be the final CL to decouple the use of 
WebViewImpl in web/ and move everything over to WebViewBase.

As before, there were a number of methods defined in WebViewImpl that needed to
be made virtual in WebViewBase and then overridden in WebViewImpl.

Also, it was not possible to use CORE_EXPORT or MODULES_EXPORT to export the
statically defined symbols from WebViewBase, so I used PLATFORM_EXPORT instead.

A number of symbols were being transitively included, so in places needed to
add include files to bring the symbols into scope.

BUG=712963
==========

to

==========
Change all test cases to use WebViewBase instead of WebViewImpl.

This CL changes all of the test cases to use the abstract WebViewBase class in
place of WebViewImpl. This should be the final CL to decouple the use of 
WebViewImpl in web/ and move everything over to WebViewBase.

As before, there were a number of methods defined in WebViewImpl that needed to
be made virtual in WebViewBase and then overridden in WebViewImpl.

Also, it was not possible to use CORE_EXPORT or MODULES_EXPORT to export the
statically defined symbols from WebViewBase, so I used PLATFORM_EXPORT instead.

A number of symbols were being transitively included, so in places needed to
add include files to bring the symbols into scope.

BUG=712963

Review-Url: https://codereview.chromium.org/2860673002
Cr-Commit-Position: refs/heads/master@{#469575}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba9abc4c70ae99cf7fc03adbd482...
==========

[commit-bot: I haz the power, 2017-05-05 02:27:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ba9abc4c70ae99cf7fc03adbd482...

[slangley, 2017-05-05 06:12:11]

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/exported/WebViewBase.h (right):

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/exported/WebViewBase.h:70: //                    
                       WebPageVisibilityState);
On 2017/05/05 at 00:54:38, haraken wrote:
> Remove this.

sorry - will do.

[dcheng, 2017-05-29 10:09:18]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-05-29 10:09:19]

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp (right):

https://codereview.chromium.org/2860673002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp:252:
WebViewBase::Create(&web_view_client_, kWebPageVisibilityStateVisible));
Btw I just came across this and found it a little confusing.

Should we have a static WebViewBase::Create() which just returns a WebViewBase*?
I think this is actually calling WebView::Create() right now.