Bug 103040 – Web Inspector: Can we provide an interface to update the "Metrics" and "Highlight Overlay" according to the change of inspected page.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Peter Wang]

Created attachment 175626 [details]
How to use Chrome to reproduce this bug

Although it can be reproduced with Chrome in remote-inspecting mode, this bug is more obvious with mobile:

(1) Open inspector on PC, turn on Element panel and select a element, we can see the "Metrics" on side-bar and "highlight Overlay" on mobile browser.
(2) Keeping the mouse unmoved on PC, rotate the device, the value of "Metrics" is unchanged and it's actually not right now, and the "highlight Overlay"on mobile browser doesn't fit the rotated screen unless you move out/in the mouse on PC.

[Peter Wang]

Created attachment 175628 [details]
Patch

[Konrad Piascik]

This patch doesn't do anything but provide the interface.  What uses the interface, please include the uses of the interface.

[Alexander Pavlov (apavlov)]

Comment on attachment 175628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175628&action=review

We do not land unused code. Please also describe how this change is intended to remedy the problem described.

As a side note, rotating a mobile device updates media query results, which is already reported through the CSS.mediaQueryResultChanged event.

> Source/WebCore/inspector/InspectorDOMAgent.h:159
> +    void documentUpated();

Typo: should be "documentUpdated"

[Pavel Feldman]

Comment on attachment 175628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175628&action=review

Pretty much every line here is wrong. Could you please start with explaining what is not working for you? Please use remote debugging scenario as an example. It is important for us to understand what you expect vs what actually happens. If it is not highlighted in the bug, we should close it as invalid.

The signal you are trying to issue is essentially a "total update". I don't think you are willing to totally update everything upon "change of the inspected page" you did not describe.

> Source/WebCore/inspector/InspectorController.h:102
> +    void documentUpdated();

InspectorController is an interface inspector exposes to the WebKit. Only control commands from WebKit are allowed to go through it, not this one.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1394
> +    m_frontend->documentUpdated();

You can't just send documentUpdated to the front-end without discarding agent state first.

>> Source/WebCore/inspector/InspectorDOMAgent.h:159
>> +    void documentUpated();
> 
> Typo: should be "documentUpdated"

InspectorDOMAgent is the only one to know when to issue documentUpdated. You can't tell it when to do it from outside. InspectorDOMAgent knows when document instance is updated based on the signals from the WebCore instrumentation (InspectorInstrumentation::)

[Peter Wang]

Created attachment 175725 [details]
Patch

[Peter Wang]

I thought you might be not interested in a special port, so just showed a common part.

Now, I've uploaded a whole patch including the part of my port to describe my purpose more clear. What I do in my port is to invoke the interface to inform Inspector update "Element Panel" when device is rotated (view port is changed).

Now, "Element Panel" is updated only when the CONTENT of document is changed. But when the device is rotated (or the window size of PC browser is changed), the ATTRIBUTE of the document's content is recalculated, I think we should do the same thing as we do when content is changed.

I'm not sure if I describe what I'm going to do clearly.

[Peter Wang]

(In reply to comment #3)
> (From update of attachment 175628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175628&action=review
> 
> We do not land unused code. Please also describe how this change is intended to remedy the problem described.
> 
> As a side note, rotating a mobile device updates media query results, which is already reported through the CSS.mediaQueryResultChanged event.
> 
> > Source/WebCore/inspector/InspectorDOMAgent.h:159
> > +    void documentUpated();
> 
> Typo: should be "documentUpdated"
I'm so sorry.

Thank you for tips, I'll check the "CSS.mediaQueryResultChanged".

[Pavel Feldman]

We should not update entire document on an attribute change. There are explicit and implicit arguments changes (setAttribute and say changing style via CSS OM). We detect first via instrumenting DOM mutation and we have special handling for the latter. What attribute of DOM changes and why in your case? I.e. what part of WebCore obsoletes the old value and computes the new one?

[Peter Wang]

(In reply to comment #8)
> We should not update entire document on an attribute change. There are explicit and implicit arguments changes (setAttribute and say changing style via CSS OM). We detect first via instrumenting DOM mutation and we have special handling for the latter. What attribute of DOM changes and why in your case? I.e. what part of WebCore obsoletes the old value and computes the new one?

For example, when mobile is rotated, the width of <div> becomes more wide, but the "Metrics" and "highlight overlay" still show the old results (refer to the attached picture2 and picture3).

[Peter Wang]

Created attachment 175734 [details]
The highlight overlay before rotating

[Peter Wang]

Created attachment 175735 [details]
After rotating, the highlight overlay still is the old size

[Peter Wang]

(In reply to comment #11)
> Created an attachment (id=175735) [details]
> After rotating, the highlight overlay still is the old size
Because Inspector recalculates the overlay only when the mouse pointer move out/in an element. But if the mouse pointer keep staying on an element, and the size of this element is changed (due to rotating), overlay has not chance to know the new size. 

And "Metrics" has the same problem.

[Pavel Feldman]

> For example, when mobile is rotated, the width of <div> becomes more wide, but the "Metrics" and "highlight overlay" still show the old results (refer to the attached picture2 and picture3).

I am sure layout takes place and layout updates the overlay. You probably use an old version of build?

> Because Inspector recalculates the overlay only when the mouse pointer move out/in an element.

This is not so.

[Alexander Pavlov (apavlov)]

(In reply to comment #13)
> > Because Inspector recalculates the overlay only when the mouse pointer move out/in an element.
> 
> This is not so.

Just verified this on the following document (ToT Chromium):

<html>
<head>
<style>

#div {
  border: 1px solid black;
  width: 200px;
  height: 100px;
}

@media (orientation: portrait) {
  #div {
    width: 100px;
    height: 200px;
  }
}
</style>
</head>
<body>
<div id="div">#div</div>
</body>
</html>

1. Enabled device metrics emulation with the resolution of 800x600, focused the "Screen resolution" width field and hovered the #div element in the DOM tree.
2. Changed width to "500" and hit Tab to apply the change, mouse pointer still over the #div element.

The #div got "rotated" along with its highlight, and the element dimensions in the tooltip got changed, too.

[Peter Wang]

(In reply to comment #13)
> > For example, when mobile is rotated, the width of <div> becomes more wide, but the "Metrics" and "highlight overlay" still show the old results (refer to the attached picture2 and picture3).
> 
> I am sure layout takes place and layout updates the overlay. You probably use an old version of build?
> 
You mean Chrome, Chrome isn't the newest version. But my port is based on the new version.

The overlay will be repainted on screen, but with the old size. I set a breakpoint on InspectorOverlay::update, nothing to invoke it when rotated.

[Alexander Pavlov (apavlov)]

(In reply to comment #15)
> (In reply to comment #13)
> > > For example, when mobile is rotated, the width of <div> becomes more wide, but the "Metrics" and "highlight overlay" still show the old results (refer to the attached picture2 and picture3).
> > 
> > I am sure layout takes place and layout updates the overlay. You probably use an old version of build?
> > 
> You mean Chrome, Chrome isn't the newest version. But my port is based on the new version.
> 
> The overlay will be repainted on screen, but with the old size. I set a breakpoint on InspectorOverlay::update, nothing to invoke it when rotated.

A device rotation should inevitably call FrameView::layout(), which invokes InspectorInstrumentation::didLayout(cookie, root), which ultimately calls InspectorOverlay::update(). Did you attach to the right (inspected page) process?

InspectorOverlay::update() always uses the up-to-date node metrics:

drawNodeHighlight()
  -> buildNodeHighlight()
    -> contentBox = renderBox->contentBoxRect();

[Pavel Feldman]

(In reply to comment #14)
> (In reply to comment #13)
> > > Because Inspector recalculates the overlay only when the mouse pointer move out/in an element.
> > 
> > This is not so.
> 
> Just verified this on the following document (ToT Chromium):

You say "Because Inspector recalculates the overlay only when the mouse pointer move out/in an element.", I say "This is not so". You give an example where media query changes. How is it relevant?

[Peter Wang]

(In reply to comment #14)
> (In reply to comment #13)
> > > Because Inspector recalculates the overlay only when the mouse pointer move out/in an element.
> > 
> > This is not so.
> 
> Just verified this on the following document (ToT Chromium):
> 
> <html>
> <head>
> <style>
> 
> #div {
>   border: 1px solid black;
>   width: 200px;
>   height: 100px;
> }
> 
> @media (orientation: portrait) {
>   #div {
>     width: 100px;
>     height: 200px;
>   }
> }
> </style>
> </head>
> <body>
> <div id="div">#div</div>
> </body>
> </html>
> 
> 1. Enabled device metrics emulation with the resolution of 800x600, focused the "Screen resolution" width field and hovered the #div element in the DOM tree.
> 2. Changed width to "500" and hit Tab to apply the change, mouse pointer still over the #div element.
> 
> The #div got "rotated" along with its highlight, and the element dimensions in the tooltip got changed, too

I've got your point now . I think there is misunderstanding, I mean  if the <div> hasn't any css specifications, how to update the "Metrics" or "highlight overlay" when it's rotated?

[Pavel Feldman]

You should not need that.

[Peter Wang]

Thanks for your time, Pavel and  Alexander. 
I see, the original design is including the rotating case, but it's decided to surpport the "@media" case only.