[DevTools] Cleanup inspector overlay and make it work with virtual viewport.

[DevTools] Cleanup inspector overlay and make it work with virtual viewport.

This also fixes "inspect element" for virtual viewport.

BUG=371843
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175109

[dgozman, 2014-05-29 13:58:12]

Could you please take a look?

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorOverlay.cpp:738:
resetData->setNumber("pageScaleFactor",
m_page->settings().pinchVirtualViewportEnabled() ? 1 :
m_page->pageScaleFactor());
I think there should be utility method instead of a lot places through blink
code with |pinchVirtualViewportEnabled() ? 1 : pageScaleFactor|.

I'm not even sure why page knows about pageScaleFactor when pinchVirtualViewport
is enabled. Will it be gone or not?

[bokan, 2014-05-29 15:09:05]

I don't know this code at all so I can't comment on the details but I've tried
the patch and it looks good.

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorOverlay.cpp:738:
resetData->setNumber("pageScaleFactor",
m_page->settings().pinchVirtualViewportEnabled() ? 1 :
m_page->pageScaleFactor());
On 2014/05/29 13:58:13, dgozman wrote:
> I think there should be utility method instead of a lot places through blink
> code with |pinchVirtualViewportEnabled() ? 1 : pageScaleFactor|.
> 
> I'm not even sure why page knows about pageScaleFactor when
pinchVirtualViewport
> is enabled. Will it be gone or not?

page->pageScaleFactor() returns the actual, correct scale factor in both cases
so I'm not sure why you need this. It's the FrameView's visibleContentScale
factor that's going away and is only used in the old path. It was redundant as
the FrameView's value was set from page->pageScaleFactor anyway.

[dgozman, 2014-05-29 15:15:08]

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorOverlay.cpp:738:
resetData->setNumber("pageScaleFactor",
m_page->settings().pinchVirtualViewportEnabled() ? 1 :
m_page->pageScaleFactor());
On 2014/05/29 15:09:06, bokan wrote:
> On 2014/05/29 13:58:13, dgozman wrote:
> > I think there should be utility method instead of a lot places through blink
> > code with |pinchVirtualViewportEnabled() ? 1 : pageScaleFactor|.
> > 
> > I'm not even sure why page knows about pageScaleFactor when
> pinchVirtualViewport
> > is enabled. Will it be gone or not?
> 
> page->pageScaleFactor() returns the actual, correct scale factor in both cases
> so I'm not sure why you need this. It's the FrameView's visibleContentScale
> factor that's going away and is only used in the old path. It was redundant as
> the FrameView's value was set from page->pageScaleFactor anyway.

We use pageScaleFactor to scale from css values (like render boxes) to the
visible page sizes. Since virtual viewport does page scaling somewhere outside
of frameview, this is required.

[pfeldman, 2014-05-29 15:20:30]

lgtm

https://codereview.chromium.org/309473002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:3314: WebMouseEvent dummyEvent;
Why do we have to do this?

[bokan, 2014-05-29 15:21:16]

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorOverlay.cpp:738:
resetData->setNumber("pageScaleFactor",
m_page->settings().pinchVirtualViewportEnabled() ? 1 :
m_page->pageScaleFactor());
On 2014/05/29 15:15:08, dgozman wrote:
> On 2014/05/29 15:09:06, bokan wrote:
> > On 2014/05/29 13:58:13, dgozman wrote:
> > > I think there should be utility method instead of a lot places through
blink
> > > code with |pinchVirtualViewportEnabled() ? 1 : pageScaleFactor|.
> > > 
> > > I'm not even sure why page knows about pageScaleFactor when
> > pinchVirtualViewport
> > > is enabled. Will it be gone or not?
> > 
> > page->pageScaleFactor() returns the actual, correct scale factor in both
cases
> > so I'm not sure why you need this. It's the FrameView's visibleContentScale
> > factor that's going away and is only used in the old path. It was redundant
as
> > the FrameView's value was set from page->pageScaleFactor anyway.
> 
> We use pageScaleFactor to scale from css values (like render boxes) to the
> visible page sizes. Since virtual viewport does page scaling somewhere outside
> of frameview, this is required.

Ok, in that case, once we remove the old path this pageScaleFactor value can go
away.

[dgozman, 2014-05-29 15:28:37]

https://codereview.chromium.org/309473002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/309473002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:3314: WebMouseEvent dummyEvent;
On 2014/05/29 15:20:30, pfeldman_ooo wrote:
> Why do we have to do this?

We want to hit exactly the same node as mouse event which caused context menu
did. There is a lot of logic in platform event builders, so it's better to reuse
it instead of following up with the changes.

[dgozman, 2014-05-29 16:44:12]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-05-29 16:46:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/309473002/1

[commit-bot: I haz the power, 2014-05-30 07:58:14]

Change committed as 175109