Bug 95080 - Web Inspector: introduce overlay page and migrate "paused in debugger" to it.
Summary: Web Inspector: introduce overlay page and migrate "paused in debugger" to it.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-27 06:11 PDT by Pavel Feldman
Modified: 2012-08-28 01:52 PDT (History)
17 users (show)

See Also:


Attachments
Patch (29.69 KB, patch)
2012-08-27 06:13 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (33.11 KB, patch)
2012-08-27 06:53 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (33.50 KB, patch)
2012-08-27 07:48 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (32.24 KB, patch)
2012-08-27 22:41 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (32.59 KB, patch)
2012-08-27 23:11 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2012-08-27 06:11:23 PDT
Separate page instance is now used to render overlay. Paused in debugger is migrated to the new overlay.
Comment 1 Pavel Feldman 2012-08-27 06:13:22 PDT
Created attachment 160705 [details]
Patch
Comment 2 Gyuyoung Kim 2012-08-27 06:17:15 PDT
Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13620108
Comment 3 kov's GTK+ EWS bot 2012-08-27 06:24:13 PDT
Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13623008
Comment 4 Early Warning System Bot 2012-08-27 06:29:43 PDT
Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13608409
Comment 5 Early Warning System Bot 2012-08-27 06:36:34 PDT
Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13609345
Comment 6 Build Bot 2012-08-27 06:44:03 PDT
Comment on attachment 160705 [details]
Patch

Attachment 160705 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13616168
Comment 7 Pavel Feldman 2012-08-27 06:53:19 PDT
Created attachment 160710 [details]
Patch
Comment 8 Andrey Kosyakov 2012-08-27 07:06:05 PDT
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.
Comment 9 Andrey Kosyakov 2012-08-27 07:15:40 PDT
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?
Comment 10 Andrey Kosyakov 2012-08-27 07:20:41 PDT
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.
Comment 11 Pavel Feldman 2012-08-27 07:48:27 PDT
Created attachment 160718 [details]
Patch
Comment 12 Yury Semikhatsky 2012-08-27 08:05:57 PDT
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.
Comment 13 Pavel Feldman 2012-08-27 22:41:44 PDT
Created attachment 160902 [details]
Patch
Comment 14 Pavel Feldman 2012-08-27 22:45:01 PDT
(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.
Comment 15 Pavel Feldman 2012-08-27 23:11:47 PDT
Created attachment 160907 [details]
Patch
Comment 16 Yury Semikhatsky 2012-08-27 23:18:32 PDT
(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.
Comment 17 Pavel Feldman 2012-08-28 01:52:07 PDT
Committed r126857: <http://trac.webkit.org/changeset/126857>