FrameTree::Find only searches relative to local frames.

FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
TBR=gene@chromium.org (got lgtm via gene1...)

Review-Url: https://codereview.chromium.org/2907663004
Cr-Commit-Position: refs/heads/master@{#476020}
Committed: https://chromium.googlesource.com/chromium/src/+/2fbbe131dc69bab2acc38c7289999cdd5e5716c8

[Łukasz Anforowicz, 2017-05-25 21:40:51]

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

[commit-bot: I haz the power, 2017-05-25 21:41:32]

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-25 21:46:08]

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

[commit-bot: I haz the power, 2017-05-25 21:46:09]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Łukasz Anforowicz, 2017-05-25 22:22:35]

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

[commit-bot: I haz the power, 2017-05-25 22:23:48]

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-25 22:42:55]

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

[commit-bot: I haz the power, 2017-05-25 22:42:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2017-05-25 22:53:04]

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

[commit-bot: I haz the power, 2017-05-25 22:54:04]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-25 23:09:12]

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

[commit-bot: I haz the power, 2017-05-25 23:10:26]

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-26 03:59:36]

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

[commit-bot: I haz the power, 2017-05-26 03:59:37]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-05-26 13:51:25]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Changes WebView::FindFrameByName so that the 2nd parameter:
  - Is a WebLocalFrame, not WebFrame
  - Is a non-optional const-ref

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.


All the existing callers of FrameTree::Find are already doing the lookup
relative lookup frames, except test code.  This CL preserves the old
behavior, by 1) doing the lookup relative to the main frame and 2)
failing safely if the main frame is remote (i.e. returning null like
the old WebView::FindFrameByName used to).

BUG=718489
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

BUG=718489
==========

[Łukasz Anforowicz, 2017-05-26 13:55:32]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

BUG=718489
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

The last change also reinforces that the lookup is done within a
browsing instance (*), not just within a frame tree associated with a
WebView.  (*) except extensions...

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

BUG=718489
==========

[Łukasz Anforowicz, 2017-05-26 13:59:07]

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

[commit-bot: I haz the power, 2017-05-26 13:59:20]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-26 14:03:43]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

The last change also reinforces that the lookup is done within a
browsing instance (*), not just within a frame tree associated with a
WebView.  (*) except extensions...

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

BUG=718489
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

The move of FindFrameByName method (including tweaks of the doc comment)
also highlights that the lookup (as implemented via FrameTree::Find)
should only look within a browsing instance, not just within the frame
tree of a given WebView.  OTOH, this comes with some disclaimers: 1)
until https://crbug.com/718489 is fixed the lookup is process-wide, and
2) some extension features require ignoring the browsing instance
boundaries.

BUG=718489
==========

[Łukasz Anforowicz, 2017-05-26 14:05:59]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (i.e. returning null like the old WebView::FindFrameByName
used to).

The move of FindFrameByName method (including tweaks of the doc comment)
also highlights that the lookup (as implemented via FrameTree::Find)
should only look within a browsing instance, not just within the frame
tree of a given WebView.  OTOH, this comes with some disclaimers: 1)
until https://crbug.com/718489 is fixed the lookup is process-wide, and
2) some extension features require ignoring the browsing instance
boundaries.

BUG=718489
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
==========

[commit-bot: I haz the power, 2017-05-26 14:19:04]

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

[commit-bot: I haz the power, 2017-05-26 14:19:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Łukasz Anforowicz, 2017-05-26 15:15:11]

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

[commit-bot: I haz the power, 2017-05-26 15:15:31]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-26 16:38:38]

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

[Łukasz Anforowicz, 2017-05-26 16:38:40]

dcheng@, could you PTAL?

This is a nice refactoring in itself, but I am mainly doing it to support a
follow-up CL, that will modify FrameTree::Find to call into a (local-frame-only)
WebFrameClient as a fallback (this will go all the way to
extensions::ExtensionFrameHelper as we've discussed yesterday).

https://codereview.chromium.org/2907663004/diff/100001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2907663004/diff/100001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:418:
WebString::FromUTF8("sub1"));
ToWebLocalFrame() above is okay, because otherwise the old code would have
returned null and ASSERT_TRUE(sub1_frame) below would have fired.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4378: ->ToWebLocalFrame()
ToWebLocalFrame above is okay, because otherwise the old code would have
returned null, trigerring ASSERT_TRUE(iframe) below.

[commit-bot: I haz the power, 2017-05-26 16:57:04]

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

[commit-bot: I haz the power, 2017-05-26 16:57:05]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-26 18:28:06]

https://codereview.chromium.org/2907663004/diff/100001/content/shell/test_run...
File content/shell/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/2907663004/diff/100001/content/shell/test_run...
content/shell/test_runner/test_runner_for_specific_view.cc:682: if
(!web_view()->MainFrame()->IsWebLocalFrame())
Is this something we'll need to fix at some point? I guess it's probably not a
new problem though...

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2021: return
WebFrame::FromFrame(result);
The original helper on WebView only returned local frames it looks like (and
returned nullptr otherwise). Do you think we should keep the same interface?

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp:69: if
(!web_view->MainFrame()->IsWebLocalFrame())
As this is a unit test, you can cheat and just use MainFrameImpl() without
checking.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4378: ->ToWebLocalFrame()
On 2017/05/26 16:38:40, Łukasz A. wrote:
> ToWebLocalFrame above is okay, because otherwise the old code would have
> returned null, trigerring ASSERT_TRUE(iframe) below.

Ditto to using MainFrameImpl() here

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:115: // browsing instance
containing this frame, looking for a frame that matches
"browsing instance" isn't quite true today, is it? We still have the fallback to
Page::ordinaryPages() AFAIK

[Łukasz Anforowicz, 2017-05-26 20:04:05]

Thanks for the feedback Daniel - can you take another look please?

https://codereview.chromium.org/2907663004/diff/100001/content/shell/test_run...
File content/shell/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/2907663004/diff/100001/content/shell/test_run...
content/shell/test_runner/test_runner_for_specific_view.cc:682: if
(!web_view()->MainFrame()->IsWebLocalFrame())
On 2017/05/26 18:28:05, dcheng wrote:
> Is this something we'll need to fix at some point? I guess it's probably not a
> new problem though...

Possibly.  For now, let me just do what other similar methods in this source
file do: CHECK(...) that main frame is local.  If any test starts to need OOPIF
support here then the CHECK(...) will hopefully quickly guide them to the
realization that OOPIF support is missing.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2021: return
WebFrame::FromFrame(result);
On 2017/05/26 18:28:05, dcheng wrote:
> The original helper on WebView only returned local frames it looks like (and
> returned nullptr otherwise). Do you think we should keep the same interface?

I think these 2 concerns (finding a frame by name in the browsing instance VS
figuring out if a frame is local) are separate and should not be shoehorned into
FindFrameByName (FindFrameByNameAndCheckIfResultIsALocalFrame?).  The callers
who need to check if the frame is local (only some of the tests AFAICT) can
always call result->IsWebLocalFrame() after calling FindFrameByName.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp:69: if
(!web_view->MainFrame()->IsWebLocalFrame())
On 2017/05/26 18:28:05, dcheng wrote:
> As this is a unit test, you can cheat and just use MainFrameImpl() without
> checking.

Done (I think, I hope I've correctly understood what you meant here).

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4378: ->ToWebLocalFrame()
On 2017/05/26 18:28:06, dcheng wrote:
> On 2017/05/26 16:38:40, Łukasz A. wrote:
> > ToWebLocalFrame above is okay, because otherwise the old code would have
> > returned null, trigerring ASSERT_TRUE(iframe) below.
> 
> Ditto to using MainFrameImpl() here

Done (I think).

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:115: // browsing instance
containing this frame, looking for a frame that matches
On 2017/05/26 18:28:06, dcheng wrote:
> "browsing instance" isn't quite true today, is it? We still have the fallback
to
> Page::ordinaryPages() AFAIK

I've added a disclaimer to the comment above.  Right now the disclaimer talks
about https://crbug.com/718489, but in the follow-up CL I'll tweak the
disclaimer to talk about WebFrameClient::FindFrame extensibility point.

[dcheng, 2017-05-26 20:16:19]

LGTM with comments addressed

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp:69: if
(!web_view->MainFrame()->IsWebLocalFrame())
On 2017/05/26 20:04:05, Łukasz A. wrote:
> On 2017/05/26 18:28:05, dcheng wrote:
> > As this is a unit test, you can cheat and just use MainFrameImpl() without
> > checking.
> 
> Done (I think, I hope I've correctly understood what you meant here).

Ah.... the problem is this test isn't plumbing things through with the right
types: it should be plumbing them through as WebViewImpl (since that's what it
starts with). I guess fixing this completely correctly is out of scope since it
touches a lot of things. Good enough for now.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:115: // browsing instance
containing this frame, looking for a frame that matches
On 2017/05/26 20:04:05, Łukasz A. wrote:
> On 2017/05/26 18:28:06, dcheng wrote:
> > "browsing instance" isn't quite true today, is it? We still have the
fallback
> to
> > Page::ordinaryPages() AFAIK
> 
> I've added a disclaimer to the comment above.  Right now the disclaimer talks
> about https://crbug.com/718489, but in the follow-up CL I'll tweak the
> disclaimer to talk about WebFrameClient::FindFrame extensibility point.

Small nit: the web name for this is "same unit of related browsing contexts", so
probably that's the terminology we should use here.

Overall thoughts: I would just have this say what it does right now (falls back
to looking at all frame trees of WebViews in the same process, with no
consideration of browsing instance) and add a TODO for what we want (we want it
to consider only frames in the same unit of related browsing contexts).

That way, it's clear how this currently behaves and how it will behave in the
future.

https://codereview.chromium.org/2907663004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4377: WebFrame* iframe =
static_cast<WebViewImpl*>(web_view_helper.WebView())
WebView() already returns a WebViewImpl, no static cast needed.

[Łukasz Anforowicz, 2017-05-26 20:34:54]

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

[commit-bot: I haz the power, 2017-05-26 20:35:48]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-26 20:51:20]

Thanks.

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2907663004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:115: // browsing instance
containing this frame, looking for a frame that matches
On 2017/05/26 20:16:19, dcheng wrote:
> On 2017/05/26 20:04:05, Łukasz A. wrote:
> > On 2017/05/26 18:28:06, dcheng wrote:
> > > "browsing instance" isn't quite true today, is it? We still have the
> fallback
> > to
> > > Page::ordinaryPages() AFAIK
> > 
> > I've added a disclaimer to the comment above.  Right now the disclaimer
talks
> > about https://crbug.com/718489, but in the follow-up CL I'll tweak the
> > disclaimer to talk about WebFrameClient::FindFrame extensibility point.
> 
> Small nit: the web name for this is "same unit of related browsing contexts",
so
> probably that's the terminology we should use here.
> 
> Overall thoughts: I would just have this say what it does right now (falls
back
> to looking at all frame trees of WebViews in the same process, with no
> consideration of browsing instance) and add a TODO for what we want (we want
it
> to consider only frames in the same unit of related browsing contexts).
> 
> That way, it's clear how this currently behaves and how it will behave in the
> future.

In the end I've stopped talking about "browsing instance" / "unit of related
browsing contexts" and just said that FindFrameByName "performs the same kind of
lookup that |window.open(..., name)| would".  This also let's me avoid having to
explain how the browsing instance boundaries are sometimes violated by
extensions (or by current process-wide lookup).

https://codereview.chromium.org/2907663004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2907663004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4377: WebFrame* iframe =
static_cast<WebViewImpl*>(web_view_helper.WebView())
On 2017/05/26 20:16:19, dcheng wrote:
> WebView() already returns a WebViewImpl, no static cast needed.

Doh - thanks for pointing this out.  Done (but still keeping the new include for
IWYU).

[Łukasz Anforowicz, 2017-05-26 20:59:01]

lukasza@chromium.org changed reviewers:
+ alexmos@chromium.org, gene@chromium.org

[Łukasz Anforowicz, 2017-05-26 20:59:02]

+alexmos@ for owners review of //content
+gene@ for owners review of //components/printing

[alexmos, 2017-05-26 22:39:53]

content/ LGTM

[commit-bot: I haz the power, 2017-05-26 22:46:56]

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

[commit-bot: I haz the power, 2017-05-26 22:46:58]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-05-30 21:22:26]

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

[commit-bot: I haz the power, 2017-05-30 21:23:09]

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-31 00:49:36]

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

[commit-bot: I haz the power, 2017-05-31 00:49:37]

Dry run: This issue passed the CQ dry run.

[gene1, 2017-05-31 20:26:54]

/printing/ LGTM

[Łukasz Anforowicz, 2017-05-31 20:27:49]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-05-31 20:27:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2907663004/#ps170001
(title: "Rebasing...")

[commit-bot: I haz the power, 2017-05-31 20:29:15]

CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-31 20:50:57]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
TBR=gene@chromium.org (got lgtm via gene1...)
==========

[commit-bot: I haz the power, 2017-05-31 21:01:01]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1496262469565640,
"parent_rev": "67a6d3fc1811bf403d56f59cf4099e3f88b68063", "commit_rev":
"fecf8ad2f40ed13213e8847056abfd3a544f4bf6"}

[commit-bot: I haz the power, 2017-05-31 21:01:12]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1496262469565640,
"parent_rev": "a708b081adfe56451e1526e6e637f7b31a2b75d0", "commit_rev":
"2fbbe131dc69bab2acc38c7289999cdd5e5716c8"}

[commit-bot: I haz the power, 2017-05-31 21:01:25]

Description was changed from

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
TBR=gene@chromium.org (got lgtm via gene1...)
==========

to

==========
FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until https://crbug.com/718489 is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG=718489
TBR=gene@chromium.org (got lgtm via gene1...)

Review-Url: https://codereview.chromium.org/2907663004
Cr-Commit-Position: refs/heads/master@{#476020}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fbbe131dc69bab2acc38c728999...
==========

[commit-bot: I haz the power, 2017-05-31 21:01:26]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/2fbbe131dc69bab2acc38c728999...

[findit-for-me, 2017-05-31 22:24:27]

Findit (https://goo.gl/kROfz5) identified this CL at revision 476020 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Łukasz Anforowicz, 2017-05-31 22:36:36]

On 2017/05/31 22:24:27, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 476020 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

I am still investigating, but I can repro the failures in
WebUIResourceBrowserTest.MediaInternals* tests before this CL (i.e. at
a708b081adfe).

[Łukasz Anforowicz, 2017-05-31 22:40:28]

On 2017/05/31 22:36:36, Łukasz A. wrote:
> On 2017/05/31 22:24:27, findit-for-me wrote:
> > Findit (https://goo.gl/kROfz5) identified this CL at revision 476020 as the
> > culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> I am still investigating, but I can repro the failures in
> WebUIResourceBrowserTest.MediaInternals* tests before this CL (i.e. at
> a708b081adfe).

https://crbug.com/728396 tracks the findit's bug.