Add a method to window.internals to enable testing of inspector highlight rects

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

[Max Vujovic]

In Bug 78037, the inspector highlight rects for SVG root elements are drawn with incorrect positions and sizes. Here is a screenshot: https://bug-78037-attachments.webkit.org/attachment.cgi?id=128501

To enable a regression test for this bug, I would like to expose the positions and sizes of the inspector highlight rects via a new method, window.internals.inspectorHighlightRects. This method would return a ClientRectList.

Originally, I wanted to create a pixel test. However, the inspector highlight rects are drawn on a page overlay layer, which DRT does not capture in its snapshot of the window. We could modify DRT for all of the platforms in order to capture the page overlay layer, but a window.internals API would be a much less expensive solution.

[Max Vujovic]

Created attachment 130240 [details]
Patch

Uploading a proposed patch that adds the window.internals.inspectorHighlightRects method and adds a test for highlight rects on a div.

[Build Bot]

Comment on attachment 130240 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 130240 [details]
Patch

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

[Max Vujovic]

Created attachment 130533 [details]
Patch for EWS bots. Do not review.

Exporting symbols for win.

[Max Vujovic]

Created attachment 130540 [details]
Patch

Exporting symbols for GTK.

[Collabora GTK+ EWS bot]

Comment on attachment 130540 [details]
Patch

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

[Max Vujovic]

Created attachment 130636 [details]
Patch

Trying GTK bot again.

[Pavel Feldman]

Comment on attachment 130636 [details]
Patch

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

This looks great, thanks for implementing it!

> LayoutTests/inspector/elements/highlight-node.html:4
> +            body {

Nit: we typically don't indent head / style tags (see other tests).

> LayoutTests/inspector/elements/highlight-node.html:46
> +                    InspectorTest.completeTest();

Nit: you should run InspectorTest.completeTest() from within the callback of the evaluate above.

[WebKit Review Bot]

Comment on attachment 130636 [details]
Patch

Clearing flags on attachment: 130636

Committed r110191: <http://trac.webkit.org/changeset/110191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tor Arne Vestbø]

Broke Qt minimal 

http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio

[Max Vujovic]

(In reply to comment #8)
> (From update of attachment 130636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130636&action=review
> 
> This looks great, thanks for implementing it!
> 
> > LayoutTests/inspector/elements/highlight-node.html:4
> > +            body {
> 
> Nit: we typically don't indent head / style tags (see other tests).
> 
> > LayoutTests/inspector/elements/highlight-node.html:46
> > +                    InspectorTest.completeTest();
> 
> Nit: you should run InspectorTest.completeTest() from within the callback of the evaluate above.

Thanks for the review, Pavel!

I'll make those changes. It looks like the patch already landed, but broke the Qt minimal build. I'll try and make another patch to fix the Qt minimal build and incorporate these changes there.

[Max Vujovic]

(In reply to comment #11)
> Broke Qt minimal 
> 
> http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio

Sorry about this!

The build error is:
"../../../../Source/WebCore/testing/Internals.cpp:348: error: 'class WebCore::Page' has no member named 'inspectorController'"

I'm guessing Qt minimal doesn't enable the inspector, and I need wrap the new inspectorHighlightRect method in a #if ENABLE(INSPECTOR) block.

I'll make these changes and put up a patch ASAP.

[Tor Arne Vestbø]

(In reply to comment #13)
> (In reply to comment #11)
> > Broke Qt minimal 
> > 
> > http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio
> 
> Sorry about this!
> 
> The build error is:
> "../../../../Source/WebCore/testing/Internals.cpp:348: error: 'class WebCore::Page' has no member named 'inspectorController'"
> 
> I'm guessing Qt minimal doesn't enable the inspector, and I need wrap the new inspectorHighlightRect method in a #if ENABLE(INSPECTOR) block.
> 
> I'll make these changes and put up a patch ASAP.

About to land this https://gist.github.com/2003704

looks okey?

[Tor Arne Vestbø]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Broke Qt minimal 
> > > 
> > > http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio
> > 
> > Sorry about this!
> > 
> > The build error is:
> > "../../../../Source/WebCore/testing/Internals.cpp:348: error: 'class WebCore::Page' has no member named 'inspectorController'"
> > 
> > I'm guessing Qt minimal doesn't enable the inspector, and I need wrap the new inspectorHighlightRect method in a #if ENABLE(INSPECTOR) block.
> > 
> > I'll make these changes and put up a patch ASAP.
> 
> About to land this https://gist.github.com/2003704
> 
> looks okey?

landed in r110202, let's see if it helps

[Max Vujovic]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #11)
> > > > Broke Qt minimal 
> > > > 
> > > > http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio
> > > 
> > > Sorry about this!
> > > 
> > > The build error is:
> > > "../../../../Source/WebCore/testing/Internals.cpp:348: error: 'class WebCore::Page' has no member named 'inspectorController'"
> > > 
> > > I'm guessing Qt minimal doesn't enable the inspector, and I need wrap the new inspectorHighlightRect method in a #if ENABLE(INSPECTOR) block.
> > > 
> > > I'll make these changes and put up a patch ASAP.
> > 
> > About to land this https://gist.github.com/2003704
> > 
> > looks okey?
> 
> landed in r110202, let's see if it helps

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Broke Qt minimal 
> > > 
> > > http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/44358/steps/compile-webkit/logs/stdio
> > 
> > Sorry about this!
> > 
> > The build error is:
> > "../../../../Source/WebCore/testing/Internals.cpp:348: error: 'class WebCore::Page' has no member named 'inspectorController'"
> > 
> > I'm guessing Qt minimal doesn't enable the inspector, and I need wrap the new inspectorHighlightRect method in a #if ENABLE(INSPECTOR) block.
> > 
> > I'll make these changes and put up a patch ASAP.
> 
> About to land this https://gist.github.com/2003704
> 
> looks okey?

Possibly okay? I'm not sure if the Internals.idl reference also needs to be wrapped in pound defines. 

#if defined(ENABLE_INSPECTOR)
            ClientRectList inspectorHighlightRects(in Document document) raises (DOMException);
#endif

However, ENABLE_INSPECTOR doesn't seem to be available in the idl.

[Max Vujovic]

(In reply to comment #16)
> Possibly okay? I'm not sure if the Internals.idl reference also needs to be wrapped in pound defines. 
> 
> #if defined(ENABLE_INSPECTOR)
>             ClientRectList inspectorHighlightRects(in Document document) raises (DOMException);
> #endif
> 
> However, ENABLE_INSPECTOR doesn't seem to be available in the idl.

Looks like wrapping the inspectorHighlightRects definition in Internals.idl with some #if/#endif wasn't necessary to fix the qt-minimal build. 

Pavel - If you know, I'm wondering if it's a good idea to wrap it in some #if/#endif?

[Max Vujovic]

Created attachment 130958 [details]
Possible qt minimal build fix

Looks like the qt-minimal build now failed with the error:
generated/JSInternals.cpp:571: error: 'class WebCore::Internals' has no member named 'inspectorHighlightRects'

Until I figure out if I can exclude lines in .idl files based on an inspector flag, I put up a possible build fix. The fix removes the #if/#endif block wrapping the inspectorHighlightRects method and moves the #if/#endif inside the method where the inspectorController is actually accessed.

I want to see if this passes some bots to make sure the other builds don't suffer.

[Max Vujovic]

Created attachment 130960 [details]
Possible qt minimal build fix

I found a similar pattern that uses #if/#endif blocks in InternalSettings.cpp that looks better. I modified my possible fix to follow that pattern. It now uses UNUSED_PARAM to avoid possible compiler errors on the minimal build. I'm running the bots on this variant of the fix.

This is how the new inspectorHighlightRect function looks:

PassRefPtr<ClientRectList> Internals::inspectorHighlightRects(Document* document, ExceptionCode& ec)
{
#if ENABLE(INSPECTOR)
    if (!document || !document->page() || !document->page()->inspectorController()) {
        ec = INVALID_ACCESS_ERR;
        return ClientRectList::create();
    }

    Highlight highlight;
    document->page()->inspectorController()->getHighlight(&highlight);
    return ClientRectList::create(highlight.quads);
#else
    UNUSED_PARAM(document);
    UNUSED_PARAM(ec);
#endif
}

This is the pattern I followed:

void InternalSettings::setInspectorResourcesDataSizeLimits(int maximumResourcesContentSize, int maximumSingleResourceContentSize, ExceptionCode& ec)
{
#if ENABLE(INSPECTOR)
    if (!page() || !page()->inspectorController()) {
        ec = INVALID_ACCESS_ERR;
        return;
    }
    page()->inspectorController()->setResourcesDataSizeLimitsFromInternals(maximumResourcesContentSize, maximumSingleResourceContentSize);
#else
    UNUSED_PARAM(maximumResourcesContentSize);
    UNUSED_PARAM(maximumSingleResourceContentSize);
    UNUSED_PARAM(ec);
#endif
}

[Max Vujovic]

Created attachment 130962 [details]
Possible qt minimal build fix

One more try. Changed the INSPECTOR not enabled case in Internals::inspectorHighlightRects to:

#else
    UNUSED_PARAM(document);
    ec = INVALID_ACCESS_ERR;
    return ClientRectList::create();
#endif

[Patrick R. Gansterer]

Remaining build fix: http://trac.webkit.org/changeset/110269

[Csaba Osztrogonác]

(In reply to comment #21)
> Remaining build fix: http://trac.webkit.org/changeset/110269

Reopen, because it broke inspector/elements/highlight-node.html on all platform:
--- /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/elements/highlight-node-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/elements/highlight-node-actual.txt 
@@ -1,7 +1,3 @@
 This test verifies the position and size of the highlight rectangles overlayed on an inspected div.
 
-margin rect is 260 x 260 at (0, 0)
-border rect is 250 x 250 at (5, 5)
-padding rect is 230 x 230 at (15, 15)
-content rect is 200 x 200 at (30, 30)

Rollout landed in http://trac.webkit.org/changeset/110280
We need a proper fix to fix !ENABLE(INSPECTOR) builds.

The commnets of landed buildfixes are absolutely misleading, because
they tried to fix !ENABLE(INSPECTOR) builds, not only Qt minimal build.

[Csaba Osztrogonác]

This rollout broke the build. :(( 

I'd love to help, but unfortunately I had no idea how to fix it properly, and I don't have time to digging it. Agreed with Patrick, we rollout out the original change and followup fixes: http://trac.webkit.org/changeset/110283

Please land it again in one patch with proper buildfixes.

[Max Vujovic]

(In reply to comment #23)
> This rollout broke the build. :(( 
> 
> I'd love to help, but unfortunately I had no idea how to fix it properly, and I don't have time to digging it. Agreed with Patrick, we rollout out the original change and followup fixes: http://trac.webkit.org/changeset/110283
> 
> Please land it again in one patch with proper buildfixes.

Csaba - Sorry for all of the confusion. I feel really bad that you had to spend time on this. In hindsight, I should've requested my original patch to be rolled out, instead of a collection of build fixes. Thank you for rolling everything out. So sorry!

[Max Vujovic]

Created attachment 131418 [details]
Patch

Uploading a patch with #if ENABLE(INSPECTOR) guards in the body of the inspectorHighlightRects method. This should work on !ENABLE(INSPECTOR) builds (such as qt-minimal).

Changed the highlight-node.html test file based on Pavel's earlier comments:
- Removed indentation of head/body/etc. tags.
- Put InspectorTest.completeTest as the callback for RuntimeAgent.evaluate.

[Max Vujovic]

Comment on attachment 131418 [details]
Patch

Passed EWS bots, setting r?, cq?

[Patrick R. Gansterer]

Comment on attachment 131418 [details]
Patch

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

> Source/WebCore/testing/Internals.idl:67
> +        ClientRectList inspectorHighlightRects(in Document document) raises (DOMException);

why no #if ENABELD_INSPECTOR around this (like the ENABLE_INPUT_COLOR 6 lines above)? IMHO that makes it more clear that the method isn't implemented.

[Yury Semikhatsky]

Comment on attachment 131418 [details]
Patch

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

>> Source/WebCore/testing/Internals.idl:67
>> +        ClientRectList inspectorHighlightRects(in Document document) raises (DOMException);
> 
> why no #if ENABELD_INSPECTOR around this (like the ENABLE_INPUT_COLOR 6 lines above)? IMHO that makes it more clear that the method isn't implemented.

This should be guarded with
#if defined(ENABLE_INSPECTOR) && ENABLE_INSPECTOR

[Yury Semikhatsky]

Comment on attachment 131418 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:342
> +#if ENABLE(INSPECTOR)

Please hide the whole method behind #if ENABLE(INSPECTOR) like with selectColorInColorChooser in this file, this way you won't need the #else section. You will have to add #if defined(ENABLE_INSPECTOR) && ENABLE_INSPECTOR to inspectorHighlightRects in the idl as well to have it compilable.

[Max Vujovic]

(In reply to comment #27)
> (From update of attachment 131418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131418&action=review
> 
> > Source/WebCore/testing/Internals.idl:67
> > +        ClientRectList inspectorHighlightRects(in Document document) raises (DOMException);
> 
> why no #if ENABELD_INSPECTOR around this (like the ENABLE_INPUT_COLOR 6 lines above)? IMHO that makes it more clear that the method isn't implemented.

Hi Patrick, Yury,

I completely agree with you. I would like to wrap the method in an #if/#endif block in Internals.idl, but something seems to be wrong with the ENABLED_INSPECTOR flag. It is always undefined in Internals.idl, even with builds that include the inspector.

I've filed a new bug on it here: Bug 80990 - ENABLE_INSPECTOR flag is unavailable in .idl files

So far, the workaround appears to have been putting an #if/#endif in the method body, which avoids using the ENABLE_INSPECTOR flag in the .idl file. You can see an example of this in InternalSettings.idl, which has no #if/#endif block, and InternalSettings.cpp, which has a method like:

void InternalSettings::setInspectorResourcesDataSizeLimits(int maximumResourcesContentSize, int maximumSingleResourceContentSize, ExceptionCode& ec)
{
#if ENABLE(INSPECTOR)
    if (!page() || !page()->inspectorController()) {
        ec = INVALID_ACCESS_ERR;
        return;
    }
    page()->inspectorController()->setResourcesDataSizeLimitsFromInternals(maximumResourcesContentSize, maximumSingleResourceContentSize);
#else
    UNUSED_PARAM(maximumResourcesContentSize);
    UNUSED_PARAM(maximumSingleResourceContentSize);
    UNUSED_PARAM(ec);
#endif
}

I used the same workaround in this patch. Do you think that this is an acceptable workaround until Bug 80990 is fixed, or do you think Bug 80990 should block this one?

[WebKit Review Bot]

Comment on attachment 131418 [details]
Patch

Clearing flags on attachment: 131418

Committed r110580: <http://trac.webkit.org/changeset/110580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

  CCLD   Programs/unittests/testwebplugindatabase
  CCLD   Programs/GtkLauncher
  CXXLD  Programs/DumpRenderTree
./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::inspectorHighlightRects(WebCore::Document*, int&): error: undefined reference to 'WebCore::ClientRectList::ClientRectList(WTF::Vector<WebCore::FloatQuad, 0u> const&)'
collect2: ld returned 1 exit status
make[1]: *** [Programs/DumpRenderTree] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
make: *** [all] Error 2


Gtk build got broken after this?

[Max Vujovic]

(In reply to comment #33)
>   CCLD   Programs/unittests/testwebplugindatabase
>   CCLD   Programs/GtkLauncher
>   CXXLD  Programs/DumpRenderTree
> ./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::inspectorHighlightRects(WebCore::Document*, int&): error: undefined reference to 'WebCore::ClientRectList::ClientRectList(WTF::Vector<WebCore::FloatQuad, 0u> const&)'
> collect2: ld returned 1 exit status
> make[1]: *** [Programs/DumpRenderTree] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make[1]: Leaving directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
> make: *** [all] Error 2
> 
> 
> Gtk build got broken after this?

I'll take a look at the build log right now.

[Max Vujovic]

(In reply to comment #34)
> (In reply to comment #33)
> >   CCLD   Programs/unittests/testwebplugindatabase
> >   CCLD   Programs/GtkLauncher
> >   CXXLD  Programs/DumpRenderTree
> > ./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::inspectorHighlightRects(WebCore::Document*, int&): error: undefined reference to 'WebCore::ClientRectList::ClientRectList(WTF::Vector<WebCore::FloatQuad, 0u> const&)'
> > collect2: ld returned 1 exit status
> > make[1]: *** [Programs/DumpRenderTree] Error 1
> > make[1]: *** Waiting for unfinished jobs....
> > make[1]: Leaving directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
> > make: *** [all] Error 2
> > 
> > 
> > Gtk build got broken after this?
> 
> I'll take a look at the build log right now.

Looks like the only the GTK 32 bit bot is failing to link.
The GTK 64 bit bots build successfully.

Help from any GTK experienced personnel would be much appreciated.

In the patch I landed, I added 5 symbols, including the ones for ClientRectList, to symbols.filter. This passed GTK EWS. I'm not sure what else needs to be done here for GTK 32 bit (but I'm still looking into it).

If this seems unfixable in a reasonable amount of time or blocks someone's work, please roll out this patch (http://trac.webkit.org/changeset/110580).

Thank you,
Max

[Max Vujovic]

So, I'm guessing the GTK 32 bit bot is missing a symbol it needs for the method:

WebCore::ClientRectList::ClientRectList(WTF::Vector<WebCore::FloatQuad, 0u> const&)

I'm not sure how to get the required symbol (short of setting up a GTK 32 bit build up).

I noticed Gustavo fixed a GTK 32 bit build here before: http://trac.webkit.org/changeset/105424/trunk/Source/autotools/symbols.filter

Interestingly, that fix also was related to method that took in a Vector.
In that fix, there was already a symbol:
_ZN7WebCore30overrideUserPreferredLanguagesERKN3WTF6VectorINS0_6StringELm0EEE;

And Gustavo added:
_ZN7WebCore30overrideUserPreferredLanguagesERKN3WTF6VectorINS0_6StringELj0EEE;

(The first symbol ends with "ELm0EEE". The second symbol ends with "ELj0EEE".)

In my issue, the symbol I have for the ClientRectList(Vector<FloatQuad>) constructor is currently:
_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELm0EEE;

Notice how this also ends with "ELm0EEE".
I'm guessing that the symbol I need to add is the same, except with a "ELj0EEE" at the end, like:

_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELj0EEE;

Maybe I should try this?

[Max Vujovic]

Created attachment 131742 [details]
Prospective build fix patch after r110580 for GTK Linux 32-bit release.

Based on the previous comment, here's a possible build fix patch. (Unfortunately, I am not certain that this patch adds the correct symbol for GTK Linux 32-bit. However, it seems likely.)

[Max Vujovic]

Comment on attachment 131742 [details]
Prospective build fix patch after r110580 for GTK Linux 32-bit release.

I've moved the prospective build fix and discussion into its own bug:
Bug 81063 - GTK 32-bit Linux Release build failing after r110580 (from bug 80338)

[Max Vujovic]

The build fix for GTK 32-bit in bug 81063 worked.
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/22282

GTK 32-bit needed that one additional symbol for the ClientRectList(Vector<FloatQuad>) constructor:
_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELj0EEE

Thanks to everyone for their help!