969306 - When getQuads lands we should remove getAdjustedQuadsPolyfill from highlighter.js & LayoutHelpers.jsm

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

When getQuads lands we should remove getAdjustedQuadsPolyfill from highlighter.js & LayoutHelpers.jsm and switch to the native API with getAdjustedQuads instead.

Comment 1

[Patrick Brosset <:pbro>]

bug 917755 just landed!

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Offsets were incorrectly calculated when zoomed or on retina screens. We need to test these carefully before removing the polyfill.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: use-getQuads-969306.patch

Removing the polyfill worked as smoothly as it could... zooming is fine on both normal and retina screens, I have also added a rotated div to browser_inspector_highlighter.js.

The biggest win here apart from speed gains from using native code is that we highlight transformed elements using a transformed highlighter.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=16d18ea2ea05

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8415904 [details] [diff] [review]
use-getQuads-969306.patch

Review of attachment 8415904 [details] [diff] [review]:
-----------------------------------------------------------------

I applied the patch and tested a few cases: transformed elements, elements within iframes, scrolling while picking. Everything worked beautifully.
Highlighting transformed elements is pretty cool too!

Comment 5

[Patrick Brosset <:pbro>]

Oh, I hadn't seen the try build results. Test browser_inspector_highlighter.js fails consistently on all platforms.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> Oh, I hadn't seen the try build results. Test
> browser_inspector_highlighter.js fails consistently on all platforms.

I expected a couple of test failures... sorry for overloading you with reviews by the way.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I am not able to reproduce these failures locally so I am creating another try run:
https://tbpl.mozilla.org/?tree=Try&rev=50a40c4c30b0

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: use-getQuads-969306.patch

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: use-getQuads-969306.patch

Fixed failing test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=9051d1aabfde

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now getting test coords using getQuads

pbrosset: Can you have a quick look over the fixed test to see if you are happy with it.

This should fix zooming differences between OSes.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=0ab59a272f7f

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8433293 [details] [diff] [review]
Now getting test coords using getQuads

Review of attachment 8433293 [details] [diff] [review]:
-----------------------------------------------------------------

Try build seems pretty green so far \o/

The changes to head.js look good to me. As discussed earlier, it's not a problem at all that we're asserting the highlighter's position using the exact same getAdjustedQuads function that we're basing it on, because the test then still ensures that the BoxModelHighlighter class does a good job at translating getAdjustedQuads' returned value into a correctly positioned set of SVG elements.
So I'm fine with this.

A test we should be writing though is something that directly tests the unaltered output of getAdjustedQuads in various situations (with zoom, scroll, iframes, ...). This would be better achieved as a unit test (I don't think xpcshell tests can access the DOM though). Anyway, this can be done in a separate bug (can you file a bug to create this test?).

I'm fine with the changes to the test code itself as well (just noted below a future problem we'll have with another patch I'm working on, but it looks like your patch will make it first anyway, so I'll handle that on my side).

::: browser/devtools/inspector/test/browser_inspector_highlighter.js
@@ +27,2 @@
>  
>    openInspector(aInspector => {

Starting around here, there's a few things we can do to make this test more readable. Something like:

Task.spawn(function*() {
  // Use selectNode, which was added some time ago and takes care of the inspector-updated event
  yield selectNode(div, inspector);

  // Then call your different test cases one by one instead of having to chain them, making them generator functions so they can themselves yield
  yield testMouseOverDivHighlights();
  yield testNonTransformedBoxModelDimensionsNoZoom();
  yield testNonTransformedBoxModelDimensionsZoomed();
  yield testMouseOverRotatedHighlights();
  ...
});

// Here is one example of a test case function
function* testNonTransformedBoxModelDimensionsNoZoom() {
  info("Highlighted the non-rotated div");
  isNodeCorrectlyHighlighted(div, "non-zoomed");
  
  let onHighlighterReady = inspector.toolbox.once("highlighter-ready");
  contentViewer = gBrowser.selectedBrowser.docShell.contentViewer
                          .QueryInterface(Ci.nsIMarkupDocumentViewer);
  contentViewer.fullZoom = 2;
  yield onHighlighterReady;
}

On that note, I just realized that my changes in bug 1014547 are going to make this test fail :(
I moved the "ready" event to the highlighter's show function rather than update where it is now, because I fixed the MozAfterPaint refresh loop (which wasn't working) and that causes update to be called a lot more than before, so we don't want to be sending "ready" event on the wire for every repaint.

I guess it will be easier to land your patch first and I'll figure out something to fix this test after this.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/c2b9502bcf60

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2b9502bcf60