Bug 95080 – Web Inspector: introduce overlay page and migrate "paused in debugger" to it.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Separate page instance is now used to render overlay. Paused in debugger is migrated to the new overlay.

[Pavel Feldman]

Created attachment 160705 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13620108

[kov's GTK+ EWS bot]

Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13623008

[Early Warning System Bot]

Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13608409

[Early Warning System Bot]

Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13609345

[Build Bot]

Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13616168

[Pavel Feldman]

Created attachment 160710 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 160710 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.h:111
> +    void drawNodeHighlight(GraphicsContext*);
> +    void drawRectHighlight(GraphicsContext*);
> +    void drawOverlayPage(GraphicsContext*);

Why did this change? Does it have to be a part of this change?

> Source/WebCore/inspector/InspectorOverlayPage.html:59
> +body {
> +  margin: 0;
> +  padding: 0;
> +}
> +
> +fill {
> +  position: absolute;
> +  top: 0;
> +  right: 0;
> +  bottom: 0;
> +  left: 0;
> +}
> +
> +.dimmed {
> +  background-color: rgba(0, 0, 0, 0.31);
> +}
> +
> +.message-line {
> +  text-align: center;
> +}
> +
> +.message-box {
> +  font-family: monospace;
> +  background-color: rgb(255, 255, 194);
> +  border: 1px solid rgb(128, 128, 128);
> +  display: inline-block;
> +  margin: 10px 0;
> +  padding: 2px 4px;
> +}

4 spaces pls.

[Andrey Kosyakov]

Comment on attachment 160710 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:604
> +    stateSaver.restore();

redundant?

> Source/WebCore/inspector/InspectorOverlay.cpp:658
> +    overlayPage()->mainFrame()->script()->evaluate(ScriptSourceCode(makeString(method, "(\"", argument, "\")")));

What if argument contains quotes?

[Andrey Kosyakov]

Comment on attachment 160710 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlayPage.html:1
> +<!--

You may want to have DOCTYPE declaration here to avoid surprises from quirks mode.

[Pavel Feldman]

Created attachment 160718 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 160718 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptController.h:29
> +#include "ScriptValue.h"

Why did this change?

> Source/WebCore/inspector/InspectorOverlay.cpp:605
> +    stateSaver.restore();

Remove this line, ~GraphicsContextStateSaver will do this for you.

> Source/WebCore/inspector/InspectorOverlay.cpp:650
> +    loader->activeDocumentLoader()->writer()->begin(KURL());

Please call begin() instead.

> Source/WebCore/inspector/InspectorOverlayPage.html:86
> +<canvas id="canvas" class="fill"></canvas>

It seems to be unused, please remove.

[Pavel Feldman]

Created attachment 160902 [details]
Patch

[Pavel Feldman]

(In reply to comment #12)
> (From update of attachment 160718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160718&action=review
> 
> > Source/WebCore/bindings/js/ScriptController.h:29
> > +#include "ScriptValue.h"
> 
> Why did this change?
> 

ScriptController's evaluate returns ScriptValue that is forward-declared. But that return value is not always used. As a result, the call site for frame->script()->evaluate("Foo") needs to include "ScriptValue.h". I thought it made ore sense to include it from the ScriptController to avoid further confusion. Rolled back since it raised questions.

> > Source/WebCore/inspector/InspectorOverlay.cpp:605
> > +    stateSaver.restore();
> 
> Remove this line, ~GraphicsContextStateSaver will do this for you.
> 

Done.

> > Source/WebCore/inspector/InspectorOverlay.cpp:650
> > +    loader->activeDocumentLoader()->writer()->begin(KURL());
> 
> Please call begin() instead.
> 

Done.

> > Source/WebCore/inspector/InspectorOverlayPage.html:86
> > +<canvas id="canvas" class="fill"></canvas>
> 
> It seems to be unused, please remove.

Done.

[Pavel Feldman]

Created attachment 160907 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 160718 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160718&action=review
> > 
> > > Source/WebCore/bindings/js/ScriptController.h:29
> > > +#include "ScriptValue.h"
> > 
> > Why did this change?
> > 
> 
> ScriptController's evaluate returns ScriptValue that is forward-declared. But that return value is not always used. As a result, the call site for frame->script()->evaluate("Foo") needs to include "ScriptValue.h". I thought it made ore sense to include it from the ScriptController to avoid further confusion. Rolled back since it raised questions.
> 
I believe we generally include headers on the caller site to avoid increasing number of #includes in the header files and so compilation time.

[Pavel Feldman]

Committed r126857: <http://trac.webkit.org/changeset/126857>