Update a few more references to WebFrame to use WebLocalFrame.

Update a few more references to WebFrame to use WebLocalFrame.

I've also updated the most straightforward changes to just remove
references to WebFrame altogether. Code that uses a mixture of both
throughout hasn't changed.

BUG=346764
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263035

[dcheng, 2014-04-09 23:14:39]

Mostly mechanical changes that will be needed to land
https://codereview.chromium.org/232133004/.

[Ilya Sherman, 2014-04-09 23:22:34]

BTW, what's a "local frame", and how does it differ from a remote frame?

https://codereview.chromium.org/231793004/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/231793004/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:27: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
Hmm, why is this the only change in this file?

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:30: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
Ditto.

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:22: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
Ditto.

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(right):

https://codereview.chromium.org/231793004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:14:
#include "third_party/WebKit/public/web/WebLocalFrame.h"
Ditto.

[dcheng, 2014-04-09 23:32:27]

That's a good question. I'll update the header comments in Blink.

A WebLocalFrame is 'local' to the process--it's hosted in the current renderer.
You can get its DOM, run script against it, etc. A remote frame is one that's
hosted in a separate renderer because it's cross-site relative to the current
frame. You can only do a small subset of operations directly on a WebRemoteFrame
(like getting its parent). All other operations would need to be proxied to the
renderer hosting it.

As for your question, the reason that only includes had to be changed in some
cases is WebLocalFrame is going to be defined in WebLocalFrame.h. I've already
updated WebDocument to return a WebLocalFrame (since by definition you can't be
dealing with a remote frame in that instance), but that led to attempts to
access an incomplete class (since WebFrame.h only forward declares it).

[Ilya Sherman, 2014-04-09 23:39:46]

On 2014/04/09 23:32:27, dcheng wrote:
> A WebLocalFrame is 'local' to the process--it's hosted in the current
renderer.
> You can get its DOM, run script against it, etc. A remote frame is one that's
> hosted in a separate renderer because it's cross-site relative to the current
> frame. You can only do a small subset of operations directly on a
WebRemoteFrame
> (like getting its parent). All other operations would need to be proxied to
the
> renderer hosting it.

That makes sense -- thanks.

> As for your question, the reason that only includes had to be changed in some
> cases is WebLocalFrame is going to be defined in WebLocalFrame.h. I've already
> updated WebDocument to return a WebLocalFrame (since by definition you can't
be
> dealing with a remote frame in that instance), but that led to attempts to
> access an incomplete class (since WebFrame.h only forward declares it).

Well, for example, form_autofill_util.cc has no references to WebLocalFrame.  It
has plenty of references to WebFrame.  Did you intend to update all of those
references to be WebLocalFrame as well?  If not, why is the new header needed?

[dcheng, 2014-04-09 23:46:07]

On 2014/04/09 23:39:46, Ilya Sherman wrote:
> On 2014/04/09 23:32:27, dcheng wrote:
> > A WebLocalFrame is 'local' to the process--it's hosted in the current
> renderer.
> > You can get its DOM, run script against it, etc. A remote frame is one
that's
> > hosted in a separate renderer because it's cross-site relative to the
current
> > frame. You can only do a small subset of operations directly on a
> WebRemoteFrame
> > (like getting its parent). All other operations would need to be proxied to
> the
> > renderer hosting it.
> 
> That makes sense -- thanks.
> 
> > As for your question, the reason that only includes had to be changed in
some
> > cases is WebLocalFrame is going to be defined in WebLocalFrame.h. I've
already
> > updated WebDocument to return a WebLocalFrame (since by definition you can't
> be
> > dealing with a remote frame in that instance), but that led to attempts to
> > access an incomplete class (since WebFrame.h only forward declares it).
> 
> Well, for example, form_autofill_util.cc has no references to WebLocalFrame. 
It
> has plenty of references to WebFrame.  Did you intend to update all of those
> references to be WebLocalFrame as well?  If not, why is the new header needed?

Ah--but it does indirectly via WebDocument. e.g. it'll do things like
WebDocument::frame()->unmarkText() (in form_autofill_util.cc). This will break
if I land https://codereview.chromium.org/232133004/ without making these fixes.

There are indeed other WebFrame references, and I imagine these will decrease or
disappear completely with time as people migrate methods off WebFrame to
WebLocalFrame. That's out of scope of the initial patch though--having the
subclass relationship landed will make it easier to parallelize the remaining
efforts.

[Ilya Sherman, 2014-04-10 00:00:11]

On 2014/04/09 23:46:07, dcheng wrote:
> On 2014/04/09 23:39:46, Ilya Sherman wrote:
> > On 2014/04/09 23:32:27, dcheng wrote:
> > > A WebLocalFrame is 'local' to the process--it's hosted in the current
> > renderer.
> > > You can get its DOM, run script against it, etc. A remote frame is one
> that's
> > > hosted in a separate renderer because it's cross-site relative to the
> current
> > > frame. You can only do a small subset of operations directly on a
> > WebRemoteFrame
> > > (like getting its parent). All other operations would need to be proxied
to
> > the
> > > renderer hosting it.
> > 
> > That makes sense -- thanks.
> > 
> > > As for your question, the reason that only includes had to be changed in
> some
> > > cases is WebLocalFrame is going to be defined in WebLocalFrame.h. I've
> already
> > > updated WebDocument to return a WebLocalFrame (since by definition you
can't
> > be
> > > dealing with a remote frame in that instance), but that led to attempts to
> > > access an incomplete class (since WebFrame.h only forward declares it).
> > 
> > Well, for example, form_autofill_util.cc has no references to WebLocalFrame.

> It
> > has plenty of references to WebFrame.  Did you intend to update all of those
> > references to be WebLocalFrame as well?  If not, why is the new header
needed?
> 
> Ah--but it does indirectly via WebDocument. e.g. it'll do things like
> WebDocument::frame()->unmarkText() (in form_autofill_util.cc). This will break
> if I land https://codereview.chromium.org/232133004/ without making these
fixes.

I see.  Tricky.

> There are indeed other WebFrame references, and I imagine these will decrease
or
> disappear completely with time as people migrate methods off WebFrame to
> WebLocalFrame. That's out of scope of the initial patch though--having the
> subclass relationship landed will make it easier to parallelize the remaining
> efforts.

Fair enough.  Does it make sense to also keep WebFrame.h included in the
meantime?

[dcheng, 2014-04-10 00:08:04]

On 2014/04/10 00:00:11, Ilya Sherman wrote:
> On 2014/04/09 23:46:07, dcheng wrote:
> > On 2014/04/09 23:39:46, Ilya Sherman wrote:
> > > On 2014/04/09 23:32:27, dcheng wrote:
> > > > A WebLocalFrame is 'local' to the process--it's hosted in the current
> > > renderer.
> > > > You can get its DOM, run script against it, etc. A remote frame is one
> > that's
> > > > hosted in a separate renderer because it's cross-site relative to the
> > current
> > > > frame. You can only do a small subset of operations directly on a
> > > WebRemoteFrame
> > > > (like getting its parent). All other operations would need to be proxied
> to
> > > the
> > > > renderer hosting it.
> > > 
> > > That makes sense -- thanks.
> > > 
> > > > As for your question, the reason that only includes had to be changed in
> > some
> > > > cases is WebLocalFrame is going to be defined in WebLocalFrame.h. I've
> > already
> > > > updated WebDocument to return a WebLocalFrame (since by definition you
> can't
> > > be
> > > > dealing with a remote frame in that instance), but that led to attempts
to
> > > > access an incomplete class (since WebFrame.h only forward declares it).
> > > 
> > > Well, for example, form_autofill_util.cc has no references to
WebLocalFrame.
> 
> > It
> > > has plenty of references to WebFrame.  Did you intend to update all of
those
> > > references to be WebLocalFrame as well?  If not, why is the new header
> needed?
> > 
> > Ah--but it does indirectly via WebDocument. e.g. it'll do things like
> > WebDocument::frame()->unmarkText() (in form_autofill_util.cc). This will
break
> > if I land https://codereview.chromium.org/232133004/ without making these
> fixes.
> 
> I see.  Tricky.
> 
> > There are indeed other WebFrame references, and I imagine these will
decrease
> or
> > disappear completely with time as people migrate methods off WebFrame to
> > WebLocalFrame. That's out of scope of the initial patch though--having the
> > subclass relationship landed will make it easier to parallelize the
remaining
> > efforts.
> 
> Fair enough.  Does it make sense to also keep WebFrame.h included in the
> meantime?

In the past, I haven't included the headers for base classes. But I don't feel
strongly one way or another, and if there is a recommended practice, I'm happy
to follow it.

[Ilya Sherman, 2014-04-10 00:11:47]

On 2014/04/10 00:08:04, dcheng wrote:
> On 2014/04/10 00:00:11, Ilya Sherman wrote:
> > On 2014/04/09 23:46:07, dcheng wrote:
> > > On 2014/04/09 23:39:46, Ilya Sherman wrote:
> > > > On 2014/04/09 23:32:27, dcheng wrote:
> > > There are indeed other WebFrame references, and I imagine these will
> decrease
> > or
> > > disappear completely with time as people migrate methods off WebFrame to
> > > WebLocalFrame. That's out of scope of the initial patch though--having the
> > > subclass relationship landed will make it easier to parallelize the
> remaining
> > > efforts.
> > 
> > Fair enough.  Does it make sense to also keep WebFrame.h included in the
> > meantime?
> 
> In the past, I haven't included the headers for base classes. But I don't feel
> strongly one way or another, and if there is a recommended practice, I'm happy
> to follow it.

It's not so much for the base class, as it is because there are WebFrame uses
that aren't WebLocalFrame.  I guess I'm fine with either option, really -- just
a little strange to remove the WebFrame.h #includes when there are still plenty
of WebFrame references in the file.  In any case, LGTM.

[jam, 2014-04-10 01:04:28]

lgtm

[Bernhard Bauer, 2014-04-10 07:52:52]

components/plugins/ LGTM.

[dcheng, 2014-04-10 08:53:02]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-04-10 08:53:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/231793004/40001

[commit-bot: I haz the power, 2014-04-10 08:53:25]

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

[commit-bot: I haz the power, 2014-04-10 08:53:26]

Failed to apply patch for
content/renderer/media/buffered_data_source_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/buffered_data_source_unittest.cc
  Hunk #3 FAILED at 37.
  Hunk #4 succeeded at 105 (offset 12 lines).
  Hunk #5 succeeded at 224 with fuzz 2 (offset 12 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
content/renderer/media/buffered_data_source_unittest.cc.rej

Patch:       content/renderer/media/buffered_data_source_unittest.cc
Index: content/renderer/media/buffered_data_source_unittest.cc
diff --git a/content/renderer/media/buffered_data_source_unittest.cc
b/content/renderer/media/buffered_data_source_unittest.cc
index
9a2500670e5d223cd9f212ba05fe98492775ee3d..450cbecf7460c94a6f2ad27be754ae22c1bd4e2e
100644
--- a/content/renderer/media/buffered_data_source_unittest.cc
+++ b/content/renderer/media/buffered_data_source_unittest.cc
@@ -13,7 +13,7 @@
 #include "media/base/mock_filters.h"
 #include "media/base/test_helpers.h"
 #include "third_party/WebKit/public/platform/WebURLResponse.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebView.h"
 
 using ::testing::_;
@@ -23,7 +23,7 @@ using ::testing::InSequence;
 using ::testing::NiceMock;
 using ::testing::StrictMock;
 
-using blink::WebFrame;
+using blink::WebLocalFrame;
 using blink::WebString;
 using blink::WebURLLoader;
 using blink::WebURLResponse;
@@ -37,7 +37,7 @@ class MockBufferedDataSource : public BufferedDataSource {
  public:
   MockBufferedDataSource(
       const scoped_refptr<base::MessageLoopProxy>& message_loop,
-      WebFrame* frame,
+      WebLocalFrame* frame,
       media::DataSourceHost* host)
       : BufferedDataSource(message_loop, frame, new media::MediaLog(), host,
                            base::Bind(&MockBufferedDataSource::set_downloading,
@@ -93,11 +93,13 @@ static const char kFileUrl[] = "file:///tmp/bar.webm";
 class BufferedDataSourceTest : public testing::Test {
  public:
   BufferedDataSourceTest()
-      : view_(WebView::create(NULL)), frame_(WebFrame::create(&client_)) {
+      : view_(WebView::create(NULL)), frame_(WebLocalFrame::create(&client_)) {
     view_->setMainFrame(frame_);
 
-    data_source_.reset(new MockBufferedDataSource(
-        message_loop_.message_loop_proxy(), view_->mainFrame(), &host_));
+    data_source_.reset(
+        new MockBufferedDataSource(message_loop_.message_loop_proxy(),
+                                   view_->mainFrame()->toWebLocalFrame(),
+                                   &host_));
   }
 
   virtual ~BufferedDataSourceTest() {
@@ -210,7 +212,7 @@ class BufferedDataSourceTest : public testing::Test {
   scoped_ptr<TestResponseGenerator> response_generator_;
   MockWebFrameClient client_;
   WebView* view_;
-  WebFrame* frame_;
+  WebLocalFrame* frame_;
 
   StrictMock<media::MockDataSourceHost> host_;
   base::MessageLoop message_loop_;

[dcheng, 2014-04-10 16:30:40]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2014-04-10 16:30:47]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-04-10 16:30:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/231793004/50001

[commit-bot: I haz the power, 2014-04-10 18:45:40]

Change committed as 263035