Fix position of dictionary pop-up for <iframe>.

Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the
LocalFrame.

BUG=640353
Committed: https://crrev.com/cf7f50ad0c94d0c134c3b6e975c5b217ac4ace30
Cr-Commit-Position: refs/heads/master@{#414814}

[EhsanK, 2016-08-25 18:01:16]

Description was changed from

==========
t cl try
Fix position of dictionary pop-up for <iframe>.

When blink converts coordinates to AppKit coordinates it changes the point's
y-coordinate to FrameView::height() - y which is correct for root frame. For
<iframe> we should still use the root frame's FrameView::height() rather than
using the <iframe>'s FrameView.

BUG=640353
==========

to

==========
Fix position of dictionary pop-up for <iframe>.

When blink converts coordinates to AppKit coordinates it changes the point's
y-coordinate to FrameView::height() - y which is correct for root frame. For
<iframe> we should still use the root frame's FrameView::height() rather than
using the <iframe>'s FrameView.

BUG=640353
==========

[EhsanK, 2016-08-25 18:01:27]

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

[commit-bot: I haz the power, 2016-08-25 18:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-08-25 18:46:42]

Description was changed from

==========
Fix position of dictionary pop-up for <iframe>.

When blink converts coordinates to AppKit coordinates it changes the point's
y-coordinate to FrameView::height() - y which is correct for root frame. For
<iframe> we should still use the root frame's FrameView::height() rather than
using the <iframe>'s FrameView.

BUG=640353
==========

to

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the LocalFrame.

BUG=640353
==========

[EhsanK, 2016-08-25 18:47:02]

Description was changed from

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the LocalFrame.

BUG=640353
==========

to

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to 
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the 
LocalFrame.

BUG=640353
==========

[commit-bot: I haz the power, 2016-08-25 19:16:58]

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

[commit-bot: I haz the power, 2016-08-25 19:16:58]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-08-25 19:27:25]

ekaramad@chromium.org changed reviewers:
+ sky@chromium.org

[EhsanK, 2016-08-25 19:27:26]

Please take a look.

[sky, 2016-08-25 20:23:49]

I'm not a good reviewer for blink code. Please pick a more local owner.

[EhsanK, 2016-08-25 20:28:38]

ekaramad@chromium.org changed reviewers:
+ dcheng@chromium.org
- sky@chromium.org

[EhsanK, 2016-08-25 20:28:38]

Daniel, could you please take a look, or suggest a different reviewer? Thanks.

[dcheng, 2016-08-25 20:41:39]

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

[dcheng, 2016-08-25 20:41:39]

bokan@ might be a good reviewer =)

[bokan, 2016-08-26 15:31:01]

Change looks fine, can we add a test?

[EhsanK, 2016-08-26 18:53:14]

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

[commit-bot: I haz the power, 2016-08-26 18:54:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-08-26 19:00:26]

Thanks David! I figured adding a test to WebViewTest right next to the original
test for WebSubstringUtil should be a good idea. This one tests for <iframe> and
will fail without the fix. PTAL

[bokan, 2016-08-26 19:10:31]

Thanks, lgtm % nits

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

https://codereview.chromium.org/2275323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3205:
URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()),
WebString::fromUTF8("visible_iframe.html"));
I don't think you have to register iframe loads but I could be wrong. See if it
works without this line.

https://codereview.chromium.org/2275323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3215: ASSERT_TRUE(!!result);
Nit: no double-bang needed. Even better: ASSERT_NE(result, nullptr)

[commit-bot: I haz the power, 2016-08-26 20:09:46]

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

[commit-bot: I haz the power, 2016-08-26 20:09:47]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-08-26 20:14:49]

Thanks David for the reviews! I will CQ after dry-run.

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

https://codereview.chromium.org/2275323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3205:
URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()),
WebString::fromUTF8("visible_iframe.html"));
On 2016/08/26 19:10:31, bokan wrote:
> I don't think you have to register iframe loads but I could be wrong. See if
it
> works without this line.

Unfortunately, I can't remove it since this CHECK will fire if I do:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi...

https://codereview.chromium.org/2275323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3215: ASSERT_TRUE(!!result);
On 2016/08/26 19:10:31, bokan wrote:
> Nit: no double-bang needed. Even better: ASSERT_NE(result, nullptr)

Thanks! Done.

[EhsanK, 2016-08-26 20:14:54]

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

[EhsanK, 2016-08-26 20:14:58]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-08-26 20:14:58]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-08-26 20:14:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2275323003/#ps40001
(title: "Addressing bokan@'s comments")

[commit-bot: I haz the power, 2016-08-26 20:16:07]

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

[bokan, 2016-08-26 20:23:19]

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

https://codereview.chromium.org/2275323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3205:
URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()),
WebString::fromUTF8("visible_iframe.html"));
On 2016/08/26 20:14:48, EhsanK wrote:
> On 2016/08/26 19:10:31, bokan wrote:
> > I don't think you have to register iframe loads but I could be wrong. See if
> it
> > works without this line.
> 
> Unfortunately, I can't remove it since this CHECK will fire if I do:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi...

Ok, it's fine as-is, thanks for trying.

[commit-bot: I haz the power, 2016-08-26 21:35:39]

Description was changed from

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to 
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the 
LocalFrame.

BUG=640353
==========

to

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to 
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the 
LocalFrame.

BUG=640353
==========

[commit-bot: I haz the power, 2016-08-26 21:35:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-26 21:38:25]

Description was changed from

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to 
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the 
LocalFrame.

BUG=640353
==========

to

==========
Fix position of dictionary pop-up for <iframe>.

In blink, we convert the string position to root frame and then convert to
AppKit coordinates by inverting y-axis. To do so, we must always use
FrameView::height() of the root frame rather than that of the
LocalFrame.

BUG=640353

Committed: https://crrev.com/cf7f50ad0c94d0c134c3b6e975c5b217ac4ace30
Cr-Commit-Position: refs/heads/master@{#414814}
==========

[commit-bot: I haz the power, 2016-08-26 21:38:26]

Patchset 3 (id:??) landed as
https://crrev.com/cf7f50ad0c94d0c134c3b6e975c5b217ac4ace30
Cr-Commit-Position: refs/heads/master@{#414814}