Rewire Android WebView's compositor changed signal.

Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955
Committed: https://crrev.com/284add6d41ef58f8b2b2574767e6b7e03ebb0923
Cr-Commit-Position: refs/heads/master@{#400499}

[hush (inactive), 2016-06-02 23:35:42]

Description was changed from

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed a RWHVA that is constructed will soon be current. This turned out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

BUG=616955
==========

to

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

BUG=616955
==========

[hush (inactive), 2016-06-02 23:36:15]

hush@chromium.org changed reviewers:
+ boliu@chromium.org

[hush (inactive), 2016-06-02 23:36:15]

Hello,
PTAL

[hush (inactive), 2016-06-02 23:38:00]

Patchset #1 (id:1) has been deleted

[boliu, 2016-06-03 02:18:20]

https://codereview.chromium.org/2036023002/diff/20001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/android...
android_webview/android_webview.gyp:339: 'browser/compositor_id.cc',
I vote for letting gyp rot

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:519: // compositor_ may be
nullptr, in which case DidInitializeCompositor()
but you don't sent compositor_ in DidInitializeCompositor though?

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:523: compositor_id_ =
compositor_id;
surely you need to set this unconditionally?

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:527: UpdateMemoryPolicy();
these two lines are new..?

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:41: struct CompositorID;
you are including the header already

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/child_frame.cc:5: #include <memory>
what's with the include change..?

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_frame_consumer.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_frame_consumer.h:19: struct CompositorID;
no need

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.cc:19: bool
CompositorIDIsDifferent::operator()(const CompositorID& lhs,
need sorting, ie this should be <, not ==

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.h:13: CompositorID(const CompositorID&
other);
if you allow copy constructor, then add the assignment operator too

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.h:19: struct CompositorIDIsDifferent {
write an actual operator? I think that should work for std::map..

https://codereview.chromium.org/2036023002/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:178:
web_contents_(std::move(web_contents)),
you need to fix declaration order, otherwise BVR destructor is going to end up
using a dangling web_contents pointer

[hush (inactive), 2016-06-09 22:43:43]

Description was changed from

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

BUG=616955
==========

to

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955
==========

[hush (inactive), 2016-06-09 22:44:23]

review

[hush (inactive), 2016-06-09 22:44:53]

https://codereview.chromium.org/2036023002/diff/20001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/android...
android_webview/android_webview.gyp:339: 'browser/compositor_id.cc',
On 2016/06/03 02:18:20, boliu wrote:
> I vote for letting gyp rot

let's just leave it as is. one day somebody will come and delete the gyp files.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:519: // compositor_ may be
nullptr, in which case DidInitializeCompositor()
On 2016/06/03 02:18:20, boliu wrote:
> but you don't sent compositor_ in DidInitializeCompositor though?
I will set it in DidInitializeCompositor in the next patch, if compositor_ ==
nullptr

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:523: compositor_id_ =
compositor_id;
On 2016/06/03 02:18:20, boliu wrote:
> surely you need to set this unconditionally?

yeah right.... I made a mistake here. The current logic does not work if
RVHChanged is fired for the new RVH before the new RVH constructs a sync
compositor.  I will fix it in the next pathc.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:527: UpdateMemoryPolicy();
On 2016/06/03 02:18:20, boliu wrote:
> these two lines are new..?

527 and 528 are new, yes.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:41: struct CompositorID;
On 2016/06/03 02:18:20, boliu wrote:
> you are including the header already

Done.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/child_frame.cc:5: #include <memory>
On 2016/06/03 02:18:20, boliu wrote:
> what's with the include change..?

because <utility> is not used by this file and std::unique_ptr is defined by
<memory>

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_frame_consumer.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_frame_consumer.h:19: struct CompositorID;
On 2016/06/03 02:18:20, boliu wrote:
> no need

Done.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.cc:19: bool
CompositorIDIsDifferent::operator()(const CompositorID& lhs,
On 2016/06/03 02:18:20, boliu wrote:
> need sorting, ie this should be <, not ==

done

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.h:13: CompositorID(const CompositorID&
other);
On 2016/06/03 02:18:20, boliu wrote:
> if you allow copy constructor, then add the assignment operator too

why? classes like content::DropData has copy constructor, but has no assignment=
overloaded.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.h:19: struct CompositorIDIsDifferent {
On 2016/06/03 02:18:20, boliu wrote:
> write an actual operator? I think that should work for std::map..

done.

https://codereview.chromium.org/2036023002/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:178:
web_contents_(std::move(web_contents)),
On 2016/06/03 02:18:20, boliu wrote:
> you need to fix declaration order, otherwise BVR destructor is going to end up
> using a dangling web_contents pointer

great.
I moved web_contents_ above bvr_

[hush (inactive), 2016-06-09 23:19:35]

hold on there are some bugs

[boliu, 2016-06-10 15:40:33]

cursory pass

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/20001/android_webview/browser...
android_webview/browser/compositor_id.h:13: CompositorID(const CompositorID&
other);
On 2016/06/09 22:44:53, hush wrote:
> On 2016/06/03 02:18:20, boliu wrote:
> > if you allow copy constructor, then add the assignment operator too
> 
> why? classes like content::DropData has copy constructor, but has no
assignment=
> overloaded.

DropData is wrong too

https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

If you define a copy or move constructor, define the corresponding assignment
operator, and vice-versa.

Also the advice given by effective c++ fwiw

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
android_webview/browser/compositor_id.h:15: bool operator==(const CompositorID&
other) const;
for binary operators, I think general preference is for "external" ones, ie
defined not in the class, but as global functions

[boliu, 2016-06-10 15:55:38]

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
android_webview/browser/compositor_id.cc:23: bool CompositorID::operator<(const
CompositorID& other) const {
Actually, overloading operator< was bad advice. Style guide says:

https://google.github.io/styleguide/cppguide.html#Operator_Overloading
For example, if you overload <, overload all the comparison operators, and make
sure < and > never return true for the same arguments.

There are still <= and >=, which are not used, but required by style guide.

So external "LessThan" might be better in this case

[hush (inactive), 2016-06-10 23:27:13]

Patchset #3 (id:60001) has been deleted

[hush (inactive), 2016-06-10 23:28:44]

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
android_webview/browser/compositor_id.cc:23: bool CompositorID::operator<(const
CompositorID& other) const {
On 2016/06/10 15:55:38, boliu wrote:
> Actually, overloading operator< was bad advice. Style guide says:
> 
> https://google.github.io/styleguide/cppguide.html#Operator_Overloading
> For example, if you overload <, overload all the comparison operators, and
make
> sure < and > never return true for the same arguments.
> 
> There are still <= and >=, which are not used, but required by style guide.
> 
> So external "LessThan" might be better in this case

i overloaded everything in the new patchset

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/40001/android_webview/browser...
android_webview/browser/compositor_id.h:15: bool operator==(const CompositorID&
other) const;
On 2016/06/10 15:40:33, boliu wrote:
> for binary operators, I think general preference is for "external" ones, ie
> defined not in the class, but as global functions

Done.

[boliu, 2016-06-13 15:30:45]

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:504: compositor_id_ =
CompositorID();
hmm, would this be more consistent that only OnRVChanged (and the constructor)
modifies compositor_id_? ie just remove this line

I think that's a better mental model, compositor_id_ always points to what
WebContents things is the active RVH, and compositor_ will point to its the sync
compositor if there is one.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:526: UpdateMemoryPolicy();
if these two lines are new, then can we add these in a separate CL?

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:530: compositor_ = nullptr;
ditto about "compositor_id_ always point to active RVH", so just remove this
else block and move the compositor_id_ assignment out of the if block

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.cc:5: #include <memory>
not needed if header includes it already

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.cc:7: #include
"android_webview/browser/child_frame.h"
style guide says the .h file of this .cc file is the first include, and has a
blank line below it

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.h:26: CompositorID compositor_id,
const &

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/compositor_frame_consumer.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/compositor_frame_consumer.h:19: struct CompositorID;
remove

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/compositor_id.cc:12:
routing_id(content::ChildProcessHost::kInvalidUniqueID) {}
does this apply to routing_id too?

either way, I think it should be possible to get rid of this constructor
altogether?

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/compositor_id.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/compositor_id.h:17: friend bool operator==(const
CompositorID& lhs, const CompositorID& rhs);
ditto about what we discussd in person

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/hardware_renderer.h:55: CompositorID compositor_id,
const&

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/render_thread_manager.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/render_thread_manager.h:32: struct CompositorID;
struct after class

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/render_thread_manager.h:68: CompositorID compositor_id,
const&

https://codereview.chromium.org/2036023002/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/native/...
android_webview/native/aw_contents.h:350: BrowserViewRenderer
browser_view_renderer_;  // Must outlive |web_contents_|.
uhh... the comment is clearly wrong now

maybe should figure out why it was added and see if it's still the case?

https://codereview.chromium.org/2036023002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/2036023002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:92: const int process_id_;
nit: move this above routing_id_

[hush (inactive), 2016-06-14 23:48:29]

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:504: compositor_id_ =
CompositorID();
On 2016/06/13 15:30:44, boliu wrote:
> hmm, would this be more consistent that only OnRVChanged (and the constructor)
> modifies compositor_id_? ie just remove this line
> 
> I think that's a better mental model, compositor_id_ always points to what
> WebContents things is the active RVH, and compositor_ will point to its the
sync
> compositor if there is one.

removed

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:526: UpdateMemoryPolicy();
On 2016/06/13 15:30:44, boliu wrote:
> if these two lines are new, then can we add these in a separate CL?

sure.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:530: compositor_ = nullptr;
On 2016/06/13 15:30:44, boliu wrote:
> ditto about "compositor_id_ always point to active RVH", so just remove this
> else block and move the compositor_id_ assignment out of the if block

let's discuss in person on this one....

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.cc:5: #include <memory>
On 2016/06/13 15:30:44, boliu wrote:
> not needed if header includes it already

removed

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.h:26: CompositorID compositor_id,
On 2016/06/13 15:30:44, boliu wrote:
> const &

Done.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/compositor_frame_consumer.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/compositor_frame_consumer.h:19: struct CompositorID;
On 2016/06/13 15:30:44, boliu wrote:
> remove

Done.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/compositor_id.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/compositor_id.cc:12:
routing_id(content::ChildProcessHost::kInvalidUniqueID) {}
On 2016/06/13 15:30:44, boliu wrote:
> does this apply to routing_id too?
> 
> either way, I think it should be possible to get rid of this constructor
> altogether?

it does apply to routing_id. Example here:
https://cs.chromium.org/chromium/src/chrome/browser/printing/printing_message...

The reason I need this constructor is that I need them to be default initialized
to an invalid value.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/hardware_renderer.h:55: CompositorID compositor_id,
On 2016/06/13 15:30:44, boliu wrote:
> const&

Done.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/render_thread_manager.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/render_thread_manager.h:32: struct CompositorID;
On 2016/06/13 15:30:44, boliu wrote:
> struct after class

Done.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/render_thread_manager.h:68: CompositorID compositor_id,
On 2016/06/13 15:30:44, boliu wrote:
> const&

Done.

https://codereview.chromium.org/2036023002/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/native/...
android_webview/native/aw_contents.h:350: BrowserViewRenderer
browser_view_renderer_;  // Must outlive |web_contents_|.
On 2016/06/13 15:30:44, boliu wrote:
> uhh... the comment is clearly wrong now
> 
> maybe should figure out why it was added and see if it's still the case?

I put it back to place. It was added so that SynchronousCompositorClient  (BVR)
outlives SynchronousCompositor. (web_contents)
It's a great guarantee to have. https://codereview.chromium.org/1385543003/

https://codereview.chromium.org/2036023002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/2036023002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:92: const int process_id_;
On 2016/06/13 15:30:44, boliu wrote:
> nit: move this above routing_id_

Done.

[boliu, 2016-06-15 00:04:02]

lgtm % comments

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.cc:7: #include
"android_webview/browser/child_frame.h"
On 2016/06/13 15:30:44, boliu wrote:
> style guide says the .h file of this .cc file is the first include, and has a
> blank line below it

not done yet :p

https://codereview.chromium.org/2036023002/diff/100001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/100001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:124: if
(!(compositor_id_.Equals(child_frame_->compositor_id)) ||
nit: extra braces not needed, and looks kinda bad

[hush (inactive), 2016-06-15 00:19:00]

I tested on popups too, and they worked.

[hush (inactive), 2016-06-15 18:10:24]

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
File android_webview/browser/child_frame.cc (right):

https://codereview.chromium.org/2036023002/diff/80001/android_webview/browser...
android_webview/browser/child_frame.cc:7: #include
"android_webview/browser/child_frame.h"
On 2016/06/15 00:04:02, boliu wrote:
> On 2016/06/13 15:30:44, boliu wrote:
> > style guide says the .h file of this .cc file is the first include, and has
a
> > blank line below it
> 
> not done yet :p

added a blank line

https://codereview.chromium.org/2036023002/diff/100001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/100001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:124: if
(!(compositor_id_.Equals(child_frame_->compositor_id)) ||
On 2016/06/15 00:04:02, boliu wrote:
> nit: extra braces not needed, and looks kinda bad

well... maybe it's just me, but I always have great trouble figuring out the
"reign" of the ! operator

[hush (inactive), 2016-06-15 18:13:25]

hush@chromium.org changed reviewers:
+ sievers@chromium.org

[hush (inactive), 2016-06-15 18:13:26]

hello Daniel,
PTAL content/

[no sievers, 2016-06-15 18:20:37]

On 2016/06/15 18:13:26, hush wrote:
> hello Daniel,
> PTAL content/

lgtm

[hush (inactive), 2016-06-15 18:23:04]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-06-15 18:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2036023002/#ps120001
(title: "comment")

[commit-bot: I haz the power, 2016-06-15 18:23:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036023002/120001

[commit-bot: I haz the power, 2016-06-15 18:38:29]

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

[commit-bot: I haz the power, 2016-06-15 18:38:31]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hush (inactive), 2016-06-16 18:06:20]

Hi Bo,
I made a change in browser_view_renderer.cc to allow it to accept null
web_contents for test usage.
(I will write some tests in a follow CL) The test does not need to mock a
webcontents object; it just need to call "RVHChanged" at appropriate times.

[boliu, 2016-06-16 18:09:28]

On 2016/06/16 18:06:20, hush wrote:
> Hi Bo,
> I made a change in browser_view_renderer.cc to allow it to accept null
> web_contents for test usage.
> (I will write some tests in a follow CL) The test does not need to mock a
> webcontents object; it just need to call "RVHChanged" at appropriate times.

Is that good enough to make the unit tests pass? If there is no WebContents,
then no compositor will ever become active?

[boliu, 2016-06-16 18:13:21]

On 2016/06/16 18:09:28, boliu wrote:
> On 2016/06/16 18:06:20, hush wrote:
> > Hi Bo,
> > I made a change in browser_view_renderer.cc to allow it to accept null
> > web_contents for test usage.
> > (I will write some tests in a follow CL) The test does not need to mock a
> > webcontents object; it just need to call "RVHChanged" at appropriate times.
> 
> Is that good enough to make the unit tests pass? If there is no WebContents,
> then no compositor will ever become active?

Plus it should be easy to fix this in the unit tests without having to modify
production code. There's already this thing:
https://cs.chromium.org/chromium/src/content/public/test/test_web_contents_fa...

[hush (inactive), 2016-06-16 20:16:14]

Patchset #7 (id:160001) has been deleted

[hush (inactive), 2016-06-16 20:20:01]

PTAL ps6.

Mocking TestWebContents would be too heavy weight. (And I ran into crashed in
web contents initialization when I just mock WebContents and BrowserContext)

Anyway, in order to unit test browser view renderer, we should just tell browser
view renderer when the RVHChanged happened, and mock the process id and routing
id of the new RVH.

[hush (inactive), 2016-06-16 20:20:20]

On 2016/06/16 20:20:01, hush wrote:
> PTAL ps6.
> 
> Mocking TestWebContents would be too heavy weight. (And I ran into crashed in
> web contents initialization when I just mock WebContents and BrowserContext)
> 
> Anyway, in order to unit test browser view renderer, we should just tell
browser
> view renderer when the RVHChanged happened, and mock the process id and
routing
> id of the new RVH.

actually ptal ps7

[hush (inactive), 2016-06-16 21:48:58]

Patchset #8 (id:200001) has been deleted

[hush (inactive), 2016-06-16 22:05:07]

PTAL

[boliu, 2016-06-16 22:13:43]

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:94: CompositorID compositor_id,
can we get rid of this parameter, and have AwContents constructor call
SetCompositorID, so there is not two ways to do the same thing

then I think AwContents changes can be cleaned up a bit too

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:32: class WebContents;
can this be removed too?

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:138: void SetCompositorID(const
CompositorID& compositor_id);
Set*Active*CompositorID?

[hush (inactive), 2016-06-16 22:43:20]

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:94: CompositorID compositor_id,
On 2016/06/16 22:13:43, boliu wrote:
> can we get rid of this parameter, and have AwContents constructor call
> SetCompositorID, so there is not two ways to do the same thing
> 
> then I think AwContents changes can be cleaned up a bit too

Done.

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:32: class WebContents;
On 2016/06/16 22:13:43, boliu wrote:
> can this be removed too?

no. It is still needed for FromWebContents and other functions

https://codereview.chromium.org/2036023002/diff/220001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:138: void SetCompositorID(const
CompositorID& compositor_id);
On 2016/06/16 22:13:43, boliu wrote:
> Set*Active*CompositorID?

Done.

[boliu, 2016-06-16 23:06:17]

lgtm

[hush (inactive), 2016-06-16 23:33:04]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-06-16 23:33:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2036023002/#ps280001
(title: "fix test for realz")

[commit-bot: I haz the power, 2016-06-16 23:33:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036023002/280001

[commit-bot: I haz the power, 2016-06-16 23:54:40]

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

[commit-bot: I haz the power, 2016-06-16 23:54:42]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[hush (inactive), 2016-06-17 02:09:55]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-06-17 02:10:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036023002/280001

[commit-bot: I haz the power, 2016-06-17 02:26:24]

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

[commit-bot: I haz the power, 2016-06-17 02:26:26]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[hush (inactive), 2016-06-17 19:46:55]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-06-17 19:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2036023002/#ps300001
(title: "Use web_contents_")

[commit-bot: I haz the power, 2016-06-17 19:47:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036023002/300001

[commit-bot: I haz the power, 2016-06-17 20:47:45]

Description was changed from

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955
==========

to

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955
==========

[commit-bot: I haz the power, 2016-06-17 20:47:47]

Committed patchset #12 (id:300001)

[commit-bot: I haz the power, 2016-06-17 20:49:47]

Description was changed from

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955
==========

to

==========
Rewire Android WebView's compositor changed signal.

Make android_webview::BrowserViewRenderer a contents::WebContentsObserver that
listens to RenderViewHostChanged.

WebView used to rely on RWHVA::SetContentViewCore call to swap compositors,
which assumed any RWHVA that is constructed will soon be current. This turned
out
to be false when WebView navigates to a page that has an SSL error (in which
case a new RVH will be created for the new domain, but the new RVH won't be
swapped).

To be able to map from RVH to the compositor contained in it, we've introduced a
CompositorID type, that combines the process_id and the routing_id of the RVH.

BUG=616955

Committed: https://crrev.com/284add6d41ef58f8b2b2574767e6b7e03ebb0923
Cr-Commit-Position: refs/heads/master@{#400499}
==========

[commit-bot: I haz the power, 2016-06-17 20:49:48]

Patchset 12 (id:??) landed as
https://crrev.com/284add6d41ef58f8b2b2574767e6b7e03ebb0923
Cr-Commit-Position: refs/heads/master@{#400499}