Replicate whether a frame is in a document or shadow tree.

Replicate whether a frame is in a document or shadow tree.

This is needed for Blink to determine whether or not a frame can be
accessed via the window indexed getters (window[num]) and named
getters (window["name"]). Previously, Blink simply inspected the
tree scope of the frame's owner element and checked that it matched
the tree scope of the parent document. This breaks down in the case
of remote frames, since the document and owner element are not
available in the same process.

Since this information is determined at frame creation and cannot
change, Blink will rely on the embedder to forward this information
through frame replication. This CL is mostly straightforward
plumbing changes, but required updating lots of call sites and
function signatures.

BUG=473518
TBR=sky,tommycli,thestig,kalman,jrummell

Committed: https://crrev.com/860817a9cd18ebf745517be58fd2ed0d937f8dcd
Cr-Commit-Position: refs/heads/master@{#331059}

[dcheng, 2015-05-18 23:19:32]

dcheng@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[dcheng, 2015-05-18 23:27:27]

I think I've fixed all the compile errors, but it's hard to say. I just found
out today that Mandoline only builds in a non-component GN build and that's
still running...

https://codereview.chromium.org/1141283002/diff/20001/components/html_viewer/...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1141283002/diff/20001/components/html_viewer/...
components/html_viewer/html_document.cc:187:
blink::WebLocalFrame::create(WebTreeScopeType::Document, this));
I plan on adding assertions, but basically, a main frame should never be in a
shadow tree.

[dcheng, 2015-05-19 00:36:25]

(Also, Blink-side patch is https://codereview.chromium.org/1141553006)

[nasko, 2015-05-19 16:02:47]

Looks good overall. Just one comment about a TODO.

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
Why would it apply only for top-level? What if we have Proxy->Proxy->Frame
nesting? Why wouldn't the middle proxy not need to know its origin? I thought we
need all of them to implement ancestorOrigins, no?

[dcheng, 2015-05-19 16:09:55]

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
On 2015/05/19 16:02:47, nasko wrote:
> Why would it apply only for top-level? What if we have Proxy->Proxy->Frame
> nesting? Why wouldn't the middle proxy not need to know its origin? I thought
we
> need all of them to implement ancestorOrigins, no?

In the case with a parent, most of this replicated state is already passed in
createRemoteChild()'s arguments: the only thing we don't pass right now is the
origin. Therefore line 94 redundantly sets the name and sandbox flags. You could
argue that we should be passing that as an argument to createRemoteChild() as
well, in which case line 94 would be completely redundant.

As a result, we'd only need to retain this for the no parent case, and we can
just move this into the if (parent_routing_id == MSG_ROUTING_NONE) block above.

[nasko, 2015-05-19 16:14:16]

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
On 2015/05/19 16:09:55, dcheng wrote:
> On 2015/05/19 16:02:47, nasko wrote:
> > Why would it apply only for top-level? What if we have Proxy->Proxy->Frame
> > nesting? Why wouldn't the middle proxy not need to know its origin? I
thought
> we
> > need all of them to implement ancestorOrigins, no?
> 
> In the case with a parent, most of this replicated state is already passed in
> createRemoteChild()'s arguments: the only thing we don't pass right now is the
> origin. Therefore line 94 redundantly sets the name and sandbox flags. You
could
> argue that we should be passing that as an argument to createRemoteChild() as
> well, in which case line 94 would be completely redundant.

I think passing all of the replication state into create* methods makes sense
and internally WebRemoteFrame can do the right thing. It will do away with the
extra call we make and code will be more streamlined. Also removes possible
issues with proxy->Init using data from the WebRemoteFrame before its
replication state is set.
 
> As a result, we'd only need to retain this for the no parent case, and we can
> just move this into the if (parent_routing_id == MSG_ROUTING_NONE) block
above.

I think we break up the replicated state into its components in a lot of places
after reading your CL. I wouldn't mind if we combine them all the time into the
struct and just pass the struct around.

[dcheng, 2015-05-19 16:18:49]

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
On 2015/05/19 16:14:16, nasko wrote:
> On 2015/05/19 16:09:55, dcheng wrote:
> > On 2015/05/19 16:02:47, nasko wrote:
> > > Why would it apply only for top-level? What if we have Proxy->Proxy->Frame
> > > nesting? Why wouldn't the middle proxy not need to know its origin? I
> thought
> > we
> > > need all of them to implement ancestorOrigins, no?
> > 
> > In the case with a parent, most of this replicated state is already passed
in
> > createRemoteChild()'s arguments: the only thing we don't pass right now is
the
> > origin. Therefore line 94 redundantly sets the name and sandbox flags. You
> could
> > argue that we should be passing that as an argument to createRemoteChild()
as
> > well, in which case line 94 would be completely redundant.
> 
> I think passing all of the replication state into create* methods makes sense
> and internally WebRemoteFrame can do the right thing. It will do away with the
> extra call we make and code will be more streamlined. Also removes possible
> issues with proxy->Init using data from the WebRemoteFrame before its
> replication state is set.

Well, we can't (easily) do it for the main frame case: the core Frame is only
initialized by the call to setMainFrame().

>  
> > As a result, we'd only need to retain this for the no parent case, and we
can
> > just move this into the if (parent_routing_id == MSG_ROUTING_NONE) block
> above.
> 
> I think we break up the replicated state into its components in a lot of
places
> after reading your CL. I wouldn't mind if we combine them all the time into
the
> struct and just pass the struct around.

It'd make the calls in unit tests look pretty ugly though. Either way, when we
add a parameter, we'll need to update a bunch of tests and updating the tests
took the longest for this patch.

[Charlie Reis, 2015-05-19 23:17:08]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2015-05-19 23:17:10]

[+jam for DEPS]

I'm a bit sad that the plumbing is so pervasive, since there are very few cases
that the scope is not the default value.  I suppose it's better to be explicit
about it and not pass the wrong thing in the shadow DOM case, though.

Also CC'ing Alex (currently OOO), since this is his area.

https://codereview.chromium.org/1141283002/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:83: "+third_party/WebKit/public/web/WebTreeScopeType.h",
Please have jam@ review new DEPS changes, but I'm guessing this is ok since it's
just an enum class.

(Same for content/common/DEPS.)

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:143:
frame_tree->AddFrame(root, process_id, 14, blink::WebTreeScopeType::Document,
I agree that these changes are a bit unwieldy for every time a new item is added
to replication state.  Would it help for AddFrame to take in a
FrameReplicationState and just be able to pass a default one?

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.cc:10: // TODO(dcheng): I'm not sure it's
ever right to not pass a tree scope type
I don't see the concern.  Are you worried that someone will call the default
constructor without thinking about whether it's a shadow DOM frame or not?

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.h:13: }  // namespace blink
nit: The common practice seems to be omitting the closing comment for these
one-line blink namespaces.  See web_contents.h, render_view_test.h,
input_event.h, etc.

[dcheng, 2015-05-20 00:09:30]

On 2015/05/19 at 23:17:10, creis wrote:
> [+jam for DEPS]
> 
> I'm a bit sad that the plumbing is so pervasive, since there are very few
cases that the scope is not the default value.  I suppose it's better to be
explicit about it and not pass the wrong thing in the shadow DOM case, though.

Most of the locations that default to document scope are inside test code.
Outside of tests, the only place that unconditionally sets scope == Document is
the root frame.

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:143:
frame_tree->AddFrame(root, process_id, 14, blink::WebTreeScopeType::Document,
On 2015/05/19 23:17:10, Charlie Reis wrote:
> I agree that these changes are a bit unwieldy for every time a new item is
added
> to replication state.  Would it help for AddFrame to take in a
> FrameReplicationState and just be able to pass a default one?

That doesn't really help us a lot in tests though, and that's where most of the
churn is.

e.g. this line would just turn into:
frame_tree->AddFrame(root, process_id, 14, FrameTreeReplicationState(...));

IMO, passing structs of params around also makes calls and the function
definition itself harder to read: you just know you have a bunch of stuff in
"params" and then you have to go find the struct definition elsewhere.

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.cc:10: // TODO(dcheng): I'm not sure it's
ever right to not pass a tree scope type
On 2015/05/19 23:17:10, Charlie Reis wrote:
> I don't see the concern.  Are you worried that someone will call the default
> constructor without thinking about whether it's a shadow DOM frame or not?

Yes. I wanted to investigate this in a follow up though.

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.h:13: }  // namespace blink
On 2015/05/19 23:17:10, Charlie Reis wrote:
> nit: The common practice seems to be omitting the closing comment for these
> one-line blink namespaces.  See web_contents.h, render_view_test.h,
> input_event.h, etc.

Done.

[jam, 2015-05-20 15:05:17]

https://codereview.chromium.org/1141283002/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:83: "+third_party/WebKit/public/web/WebTreeScopeType.h",
On 2015/05/19 23:17:10, Charlie Reis wrote:
> Please have jam@ review new DEPS changes, but I'm guessing this is ok since
it's
> just an enum class.
> 
> (Same for content/common/DEPS.)

yep fine, as long as the comment in line 40 is observed which it is.

[Charlie Reis, 2015-05-20 21:38:40]

Thanks.  Some more discussion below.  I'd like to resolve some of these TODOs if
we can.

On 2015/05/20 00:09:30, dcheng wrote:
> On 2015/05/19 at 23:17:10, creis wrote:
> > [+jam for DEPS]
> > 
> > I'm a bit sad that the plumbing is so pervasive, since there are very few
> cases that the scope is not the default value.  I suppose it's better to be
> explicit about it and not pass the wrong thing in the shadow DOM case, though.
> 
> Most of the locations that default to document scope are inside test code.

I guess that's not so bad.

> Outside of tests, the only place that unconditionally sets scope == Document
is
> the root frame.
> 

Admittedly that includes not just FrameTree, but also WebViewPlugin and
PrintWebViewHelper.

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:143:
frame_tree->AddFrame(root, process_id, 14, blink::WebTreeScopeType::Document,
On 2015/05/20 00:09:29, dcheng wrote:
> On 2015/05/19 23:17:10, Charlie Reis wrote:
> > I agree that these changes are a bit unwieldy for every time a new item is
> added
> > to replication state.  Would it help for AddFrame to take in a
> > FrameReplicationState and just be able to pass a default one?
> 
> That doesn't really help us a lot in tests though, and that's where most of
the
> churn is.
> 
> e.g. this line would just turn into:
> frame_tree->AddFrame(root, process_id, 14, FrameTreeReplicationState(...));

That's why I said "pass in a default one."  Almost all of these tests just use
the default values for each of the replication state parameters, which the
zero-argument FrameReplicationState() would give you for free.  No changes to
most of the tests would be required each time we add a new flag.

> IMO, passing structs of params around also makes calls and the function
> definition itself harder to read: you just know you have a bunch of stuff in
> "params" and then you have to go find the struct definition elsewhere.

That makes sense when there's just a few parameters, but it gets unwieldy fast. 
I think we're going to need something that scales for FrameReplicationState, but
Alex would have a better sense.

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.cc:10: // TODO(dcheng): I'm not sure it's
ever right to not pass a tree scope type
On 2015/05/20 00:09:30, dcheng wrote:
> On 2015/05/19 23:17:10, Charlie Reis wrote:
> > I don't see the concern.  Are you worried that someone will call the default
> > constructor without thinking about whether it's a shadow DOM frame or not?
> 
> Yes. I wanted to investigate this in a follow up though.

Can we answer it now?  I feel like this is likely to end up as a permanent TODO
otherwise, without a bug number or reason to come back to it.

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
Daniel/Nasko: What's the outcome of your discussion here?

[dcheng, 2015-05-21 18:16:16]

On 2015/05/20 at 21:38:40, creis wrote:
> On 2015/05/20 00:09:30, dcheng wrote:
> > Outside of tests, the only place that unconditionally sets scope == Document
is
> > the root frame.
> > 
> 
> Admittedly that includes not just FrameTree, but also WebViewPlugin and
PrintWebViewHelper.

PrintWebViewHelper (should) go away though.

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:143:
frame_tree->AddFrame(root, process_id, 14, blink::WebTreeScopeType::Document,
On 2015/05/20 21:38:40, Charlie Reis wrote:
> On 2015/05/20 00:09:29, dcheng wrote:
> > On 2015/05/19 23:17:10, Charlie Reis wrote:
> > > I agree that these changes are a bit unwieldy for every time a new item is
> > added
> > > to replication state.  Would it help for AddFrame to take in a
> > > FrameReplicationState and just be able to pass a default one?
> > 
> > That doesn't really help us a lot in tests though, and that's where most of
> the
> > churn is.
> > 
> > e.g. this line would just turn into:
> > frame_tree->AddFrame(root, process_id, 14, FrameTreeReplicationState(...));
> 
> That's why I said "pass in a default one."  Almost all of these tests just use
> the default values for each of the replication state parameters, which the
> zero-argument FrameReplicationState() would give you for free.  No changes to
> most of the tests would be required each time we add a new flag.
> 

Sure, but AddFrame() doesn't need the origin, which FrameReplicationState
specifies. It seems weird to pass the entire FrameReplicationState along when
not all fields are needed.

> > IMO, passing structs of params around also makes calls and the function
> > definition itself harder to read: you just know you have a bunch of stuff in
> > "params" and then you have to go find the struct definition elsewhere.
> 
> That makes sense when there's just a few parameters, but it gets unwieldy
fast. 
> I think we're going to need something that scales for FrameReplicationState,
but
> Alex would have a better sense.

At most, I expect that we'll need to replicate one or two more things, so I
don't expect this to be a problem in practice.

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
File content/common/frame_replication_state.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/common/frame_re...
content/common/frame_replication_state.cc:10: // TODO(dcheng): I'm not sure it's
ever right to not pass a tree scope type
On 2015/05/20 21:38:40, Charlie Reis wrote:
> On 2015/05/20 00:09:30, dcheng wrote:
> > On 2015/05/19 23:17:10, Charlie Reis wrote:
> > > I don't see the concern.  Are you worried that someone will call the
default
> > > constructor without thinking about whether it's a shadow DOM frame or not?
> > 
> > Yes. I wanted to investigate this in a follow up though.
> 
> Can we answer it now?  I feel like this is likely to end up as a permanent
TODO
> otherwise, without a bug number or reason to come back to it.

While I personally prefer to see this explicitly specified, I guess it's fine in
all the current call sites.

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
On 2015/05/20 21:38:40, Charlie Reis wrote:
> Daniel/Nasko: What's the outcome of your discussion here?

This is basically a TODO for future cleanup. This is cleanup that would deferred
until the Chrome/Blink merge. I've updated the comment to reflect this.

[nasko, 2015-05-21 22:57:20]

LGTM

[Charlie Reis, 2015-05-21 23:18:18]

Ok, LGTM with one nit.

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1141283002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:143:
frame_tree->AddFrame(root, process_id, 14, blink::WebTreeScopeType::Document,
On 2015/05/21 18:16:16, dcheng wrote:
> On 2015/05/20 21:38:40, Charlie Reis wrote:
> > On 2015/05/20 00:09:29, dcheng wrote:
> > > On 2015/05/19 23:17:10, Charlie Reis wrote:
> > > > I agree that these changes are a bit unwieldy for every time a new item
is
> > > added
> > > > to replication state.  Would it help for AddFrame to take in a
> > > > FrameReplicationState and just be able to pass a default one?
> > > 
> > > That doesn't really help us a lot in tests though, and that's where most
of
> > the
> > > churn is.
> > > 
> > > e.g. this line would just turn into:
> > > frame_tree->AddFrame(root, process_id, 14,
FrameTreeReplicationState(...));
> > 
> > That's why I said "pass in a default one."  Almost all of these tests just
use
> > the default values for each of the replication state parameters, which the
> > zero-argument FrameReplicationState() would give you for free.  No changes
to
> > most of the tests would be required each time we add a new flag.
> > 
> 
> Sure, but AddFrame() doesn't need the origin, which FrameReplicationState
> specifies. It seems weird to pass the entire FrameReplicationState along when
> not all fields are needed.

Ah.  Yes, it would be unfortunate to have to pass in more than is needed just to
make it easier to call in tests.

> 
> > > IMO, passing structs of params around also makes calls and the function
> > > definition itself harder to read: you just know you have a bunch of stuff
in
> > > "params" and then you have to go find the struct definition elsewhere.
> > 
> > That makes sense when there's just a few parameters, but it gets unwieldy
> fast. 
> > I think we're going to need something that scales for FrameReplicationState,
> but
> > Alex would have a better sense.
> 
> At most, I expect that we'll need to replicate one or two more things, so I
> don't expect this to be a problem in practice.

That's seems overly optimistic to me, but I'll defer for now.  :)

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/60001/content/renderer/render...
content/renderer/render_frame_proxy.cc:93: // frame proxy?
On 2015/05/21 18:16:16, dcheng wrote:
> On 2015/05/20 21:38:40, Charlie Reis wrote:
> > Daniel/Nasko: What's the outcome of your discussion here?
> 
> This is basically a TODO for future cleanup. This is cleanup that would
deferred
> until the Chrome/Blink merge. I've updated the comment to reflect this.

Acknowledged.

https://codereview.chromium.org/1141283002/diff/80001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/80001/content/renderer/render...
content/renderer/render_frame_proxy.cc:94: // createLocalChild(). We should
update the Bilnk interface so it also takes
nit: Blink

[dcheng, 2015-05-22 01:24:13]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-05-22 01:24:14]

https://codereview.chromium.org/1141283002/diff/80001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1141283002/diff/80001/content/renderer/render...
content/renderer/render_frame_proxy.cc:94: // createLocalChild(). We should
update the Bilnk interface so it also takes
On 2015/05/21 23:18:17, Charlie Reis wrote:
> nit: Blink

Dnoe.

[dcheng, 2015-05-22 01:24:14]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1141283002/#ps100001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-05-22 01:26:46]

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

[commit-bot: I haz the power, 2015-05-22 01:40:25]

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

[commit-bot: I haz the power, 2015-05-22 01:40:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2015-05-22 01:56:33]

dcheng@chromium.org changed reviewers:
+ jrummell@chromium.org, kalman@chromium.org, sky@chromium.org,
thestig@chromium.org, tommycli@chromium.org

[dcheng, 2015-05-22 01:56:34]

TBRing people for mechanical changes:

sky@ for components/html_viewer
tommycli@ for components/plugins
thestig@ for components/printing
kalman@ for extensions
jrummell@ for media

[dcheng, 2015-05-22 01:57:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-05-22 01:58:24]

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

[Lei Zhang, 2015-05-22 01:59:40]

lgtm

https://codereview.chromium.org/1141283002/diff/100001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/1141283002/diff/100001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:590: virtual
blink::WebFrame* createChildFrame(
random comment: should these be override?

[dcheng, 2015-05-22 02:01:00]

https://codereview.chromium.org/1141283002/diff/100001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/1141283002/diff/100001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:590: virtual
blink::WebFrame* createChildFrame(
On 2015/05/22 01:59:40, Lei Zhang wrote:
> random comment: should these be override?

We generally avoid marking Blink overrides with 'override', since it makes
multiway patches much harder.

This rule can (and probably will) change once the two repos are merged.

[commit-bot: I haz the power, 2015-05-22 03:17:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-22 03:17:59]

Patchset 6 (id:??) landed as
https://crrev.com/860817a9cd18ebf745517be58fd2ed0d937f8dcd
Cr-Commit-Position: refs/heads/master@{#331059}