Closed Bug 985597 Opened 10 years ago Closed 10 years ago

Use the BoxModelHighlighter on all targets

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 36

People

(Reporter: Optimizer, Assigned: pbro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files, 28 obsolete files)

bug985597-filter-paint-updates v3.patch
11.61 KB, patch
miker
: review+
Details | Diff | Splinter Review
bug985597-dont-highlight-hidden-nodes v1.patch
1.29 KB, patch
miker
: review+
Details | Diff | Splinter Review
bug985597-boxmodelhighlighter-in-canvasframe-tests v6.patch
200.29 KB, patch
miker
: review+
Details | Diff | Splinter Review
bug985597-boxmodelhighlighter-in-canvasframe v15.patch
72.80 KB, patch
pbro
: review+
Details | Diff | Splinter Review
bug985597-layout-view-tests v1.patch
24.65 KB, patch
miker
: review+
Details | Diff | Splinter Review 
Right now, we have box model highlighter , which is awesome. Sadly, it does not work with remote targets, even if they have the box model highlighter. You can easily see this in an e10s window. or Browser Toolbox.

We have in total 3 highlighters (well 4, but lets skip the very first pink bordered Fx OS only highlighter):

1. Red dotted, for browsers having non-remote highlighters (pre bug 916443)
2. Black dotted with infobar (post bug 916443)
3. Box Model Highlighter (post bug 663778)

We have trait `highlightable` to detect the existence of the second highlighter, but sadly, no trait was added for box model highlighter.

We should first add a trait for box model highlighter and also get it uplifted to aurora.

Secondly, just like the existence of `highlightable` trait is checked in toolbox.js (get isRemoteHighlightable()), we should check for existence of box model highlighter and use that.

There seems to be another issue though. Even though in an e10s window case, `highlightable` trait should be present, the highlighter being used was such that the `highlightable` is not present. Patrick, any idea ?
Flags: needinfo?(pbrosset)
(In reply to Girish Sharma [:Optimizer] from comment #0)
> Right now, we have box model highlighter , which is awesome. Sadly, it does
> not work with remote targets, even if they have the box model highlighter.
> You can easily see this in an e10s window. or Browser Toolbox.
I haven't tested in e10s mode, but my understanding is that, in this mode, the window has no parent we can use to attach the box-model highlighter, hence we use the outline trick to highlight there.
In the browser toolbox, I tried for some time to have the box model highlighter working, but finally gave up. See https://bugzilla.mozilla.org/show_bug.cgi?id=959076#c4

> We have in total 3 highlighters (well 4, but lets skip the very first pink
> bordered Fx OS only highlighter):
> 
> 1. Red dotted, for browsers having non-remote highlighters (pre bug 916443)
Correct
> 2. Black dotted with infobar (post bug 916443)
> 3. Box Model Highlighter (post bug 663778)
Well yes, but 3 replaces 2, you can't have both in the same release. 3 is in Aurora now and 2 will disappear little by little.

> We have trait `highlightable` to detect the existence of the second
> highlighter, but sadly, no trait was added for box model highlighter.
> 
> We should first add a trait for box model highlighter and also get it
> uplifted to aurora.
> 
> Secondly, just like the existence of `highlightable` trait is checked in
> toolbox.js (get isRemoteHighlightable()), we should check for existence of
> box model highlighter and use that.
Hmm, I think the highlightable trait is here for both 2 and 3, and again, 3 replaces 2. I don't think we need a new trait for the box-model highlighter. This trait is only here to say that the server has the highlighter actor. Then, once the client knows it has it, it just asks it to highlight when needed, and it's then up to the server to decide which highlighter to use: either the complex, XUL-based, box-model highlighter, or the red ouline if browser-toolbox, or fxos, or e10s.

> There seems to be another issue though. Even though in an e10s window case,
> `highlightable` trait should be present, the highlighter being used was such
> that the `highlightable` is not present. Patrick, any idea ?
If the trait is present and the highlighter was the red outline, then it means the highlighter actor detected that the box-model highlighter could not be used, because there was no XUL parent to attach it to.

I think we should use this bug rather to choose a new way of drawing the highlighter. It's just too frustrating not being able to have the nice box-model highlighter in fennec, fxos, e10s and browser-toolbox:

- appending elements to the target's parent gives us all the flexibility we need to draw complex highlighters, but is not an option in fennec, fxos and e10s
- appending elements in-content (in the page) would work great too, but we shouldn't be impacting the content page when highlighting
- the only other option I see is what we've been discussing for some time: drawing at the C++ level. I'll try and contact people from core:layout to get some discussion started.
Flags: needinfo?(pbrosset)
Yeah. It would be really sad that after say 2 releases, box model highlighter stops working :(. As per my knowledge, e10s would be enabled by default soon.
See this thread:
https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/fiWpoAGnc8Q

Seems like there will soon be a way to include anonymous content in the root canvas frame which would give us the ability to simply move our current highlighter over into it instead of having it inside the browser XUL structure.
Bug 924692 is where the anonymous content in canvas-frame thing is being worked on.
Depends on: 924692
Blocks: firebug-gaps
Bug 924692 just landed, awesome.

Now, a couple of things are missing:
- the code that creates the anonymous content seems pretty specifically tied to the touch caret and it doesn't seem possible as of now to insert any content,
- this can't be accessed by privileged JS code yet. 

Now depending on bug 1020244.
Depends on: 1020244
OS: Windows 7 → All
Hardware: x86_64 → All
Blocks: dte10s
I'm working on bug 1020244 now. We should have a new container for the boxmodelhighlighter that works on all targets soon-ish.
Summary: Inspector should check for the latest type of highlighter available on remote targets → Use the BoxModelHighlighter on all targets
This needs the 2 patches in bug 1020244 to be applied first.
It sort of works, but it's more a proof of concept than anything. It will badly crash if you navigate to another page, or open the browser toolbox.
Having said that, it does work also in e10s windows \o/.
Since this is just a proof of concept, I haven't taken the time to migrate the boxmodelhighlighter, so for now this is just a using a new, simple rectangle, highlighter.
Still a WIP, but almost works (all types of highlighters do work too)!!

The patch for required bug 1020244 isn't yet finished, but works well enough to support this patch.

Here is a list of tasks to complete before this can start being reviewed:

- where should css go? For now it's in ua.css, but we should try and dynamically load it in canvasFrame too
- should test on b2g/fennec
- doesn't work on XUL windows
- rewrite all highlighter tests :( 
- re-appending the highlighter after navigating to a new window doesn't work yet
- the nodeinfobar is broken
- many code cleanups
Assignee: nobody → pbrosset
Attachment #8477390 - Attachment is obsolete: true
Getting there ...
Progress since last patch:
- main styling of the highlighter (since the highlighter is now HTML instead of XUL, this required a lot of tweaks)
- works after page navigation
- nodeinfobar is almost positioned correctly now
- works on b2g! https://www.youtube.com/watch?v=BJHQtxea9T4 . In e10s too.

Left to do:
- Move the css to a better place
- Test on fennec
- rewrite all highlighter tests :( 
- finish fixing the nodeinfobar positioning
Attachment #8484845 - Attachment is obsolete: true
I'm unsure about where the CSS code to style the highlighter should go.

The highlighter requires a few CSS rules to style the dashed guides, background color of region boxes, the node infobar, etc. (as seen in this screenshot: https://bug1020244.bugzilla.mozilla.org/attachment.cgi?id=8443340).

For now, the only solution I found was to just insert the CSS code in ua.css but I doubt this is what we want in the end.

I've tried inserting a <link> element into the nsCanvasFrame anonymous container, using the API being developed in bug 1020244, that pointed to chrome://.../myHighlighterStylesheet.css (that didn't work but tbh I haven't yet investigated why).

Alternativel, using the same API, I could call getStyleForElement for each of the elements of the highlighter I need to style and go from there, but that means the styling will be done in javascript rather than css.

What do you think could be a valid solution for this Roc?
Flags: needinfo?(roc)
Two possibilities:
-- Just use inline style everywhere.
-- Add a <style scoped> element with your styles to the anonymous content you're injecting.

We don't really want a global stylesheet since that could affect or be detected by Web page content.
Flags: needinfo?(roc)
Making progress. Main changes in this patch:

- SimpleOutlineHighlighter is used for XUL windows,
- when navigating to a different window type (from an mozilla.org to about:newtab for instance, or opposite direction) the previous highlighter is removed and a new one constructed (either simple outline or box model, depending on the window),

The css style for the highlighter is still in ua.css though, I can't get it to insert properly in the canvasframe for some reason. I've tried inserting a <link>, <style>, <style scoped>, tried using a @import with the stylesheet URL, but also tried fetching the content and writing it inside the <style>. Everytime the insert seems to work (no error), but the style isn't applied.
Attachment #8485729 - Attachment is obsolete: true
Nodeinfobar positioning is now better.
Highlighter css is still in ua.css, I need to figure out why inserting the stylesheet doesn't work.
Next step: fixing all the tests!
Attachment #8491459 - Attachment is obsolete: true
Test patch, not ready yet.
Still WIP, I'm slowly going through all tests, making sure those that check the highlighter work, and also making them e10s ready, as well as use the new 'add_task' function (see bug 1075319).
Attachment #8501705 - Attachment is obsolete: true
For info, bug 1074836 is the original bug for making the inspector tests e10s ready. I expect to have migrated most tests in this bug though. So we can use 1074836 to finish migrating those that are left.
Getting there ... about 10 tests remaining before I can call this patch version 1.
Attachment #8501859 - Attachment is obsolete: true
v4 of the main highlighter patch, minor updates.
Also changed the way the highlighter is re-created after page navigation (indeed, on page navigation the document gets recreated, so the canvasframe doesn't contain the highlighter anymore, so now highlighters have to define a markup builder function that will get called automatically on page navigation).
Attachment #8498988 - Attachment is obsolete: true
Yay, done with the test patch. All tests now pass again with the new highlighter.
I have left the skip-if e10s condition in there but my changes make the tests that much easier to make e10s ready, so bug 1074836 should be easier to deal with.
Attachment #8502401 - Attachment is obsolete: true
v5 un-breaks the css transform highlighter.
Attachment #8502503 - Attachment is obsolete: true
Updated after the platform API changes in bug 1020244
Attachment #8502511 - Attachment is obsolete: true
Moved all highlighter tests in toolkit/devtools/server/test/ to browser/devtools/inspector/test so that all highlighter-related tests are in one place and can all use the new helper functions introduced.
Attachment #8502504 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Two possibilities:
> -- Just use inline style everywhere.
> -- Add a <style scoped> element with your styles to the anonymous content
> you're injecting.
> 
> We don't really want a global stylesheet since that could affect or be
> detected by Web page content.

I have tried adding a <style scoped> in the content I'm inserting in the native anonymous container via Document::InsertAnonymousContent but that didn't work.
What seems to be happening is:
- the style node actually gets inserted
- but the associated stylesheet gets applied to the main content document, not to the native anonymous elements in the canvasFrame.

If I add "* {background: red !important;}" to my styles for instance, I see this being applied to content elements but not to my highlighter (of course that's if I remove the scoped attribute, otherwise the style doesn't apply anywhere).

The same thing seems to occur with a <link> node.

Looking at the code, I see that native anonymous elements get styled by what I think is user-agent stylesheets (like layout/style/forms.css), loaded by nsLayoutStylesheetCache and then appended in the content document (?) by nsDocumentViewer::CreateStyleSet.
I don't see any code that tries to insert stylesheets inside native anonymous elements like what I'm trying to do here. 

Once way that does work is, in js:

  let {Style} = require("sdk/stylesheet/style");
  let {attach} = require("sdk/content/mod");
  let style = Style({source: "my stylesheet content", type: "agent"});
  attach(style, win);

Roc, I'm a bit lost here, I don't know enough about Gecko to understand if what I'm trying to do is incorrect, or just not working by design (and if it's the case, do we want to change it).
Thanks for your help.
Flags: needinfo?(roc)
The platform API has changed in bug 1020244's patches. Here's a new highlighter patch updated accordingly.
Attachment #8505445 - Attachment is obsolete: true
An unrelated (and buggy) changed made it into the previous patch. This removed the change.
Attachment #8506922 - Attachment is obsolete: true
Minor update after I found another error when destroying a highlighter instance.
Attachment #8506927 - Attachment is obsolete: true
And fixed various errors in the tests.

Before going any further, I'd like some feedback on the way the new highlighter tests work: Brian, Mike, do these changes make sense?

- there is no way to access the highlighter DOM node now
- the only thing we can do is get/set attributes or textcontent via a chrome-only document api
- to do this, we need to get access to the highlighter actor instance
- I've added a frame script that provides the needed facility, look at doc_frame_script.js in this patch, in particular the "Test:GetHighlighterAttribute" message handler
- it works by requiring the highlighter module and from there accesses actor instances
- actor instances are being set and removed when highlighters are init'ed and destroyed, see highlighter.js in this patch

Let me know what you think of this approach.
Attachment #8505446 - Attachment is obsolete: true
Attachment #8506994 - Flags: feedback?(mratcliffe)
Attachment #8506994 - Flags: feedback?(bgrinstead)
Updated obsolete comment.
Attachment #8506994 - Attachment is obsolete: true
Attachment #8506994 - Flags: feedback?(mratcliffe)
Attachment #8506994 - Flags: feedback?(bgrinstead)
Attachment #8506997 - Flags: feedback?(mratcliffe)
Attachment #8506997 - Flags: feedback?(bgrinstead)
Blocks: 1084442
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> Roc, I'm a bit lost here, I don't know enough about Gecko to understand if
> what I'm trying to do is incorrect, or just not working by design (and if
> it's the case, do we want to change it).

We could probably make <style scoped> work but if it's just as easy for you to use inline style, can you do that?
Flags: needinfo?(roc)
If you need to use <style scoped> or similar (because you need selectors or something), file a new bug on <style scoped> not working inside an anonymous element and request help from heycam.
Comment on attachment 8506997 [details] [diff] [review]
bug985597-boxmodelhighlighter-in-canvasframe-tests v4.patch

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

::: browser/devtools/inspector/test/browser_inspector_highlighter-02.js
@@ +25,3 @@
>  
>    info("Selecting the rotated DIV");
> +  yield selectAndHighlightNode("#rotated-div", inspector);

This is the only test we have that checks if zoomed pages are correctly highlighted so we shouldn't remove that part of the test.

::: browser/devtools/inspector/test/browser_inspector_highlighter-options.js
@@ +136,5 @@
> +add_task(function*() {
> +  let {inspector, toolbox} = yield openInspectorForURL(TEST_URL);
> +
> +  let divFront = yield getNodeFront("div", inspector);
> +  

NIT: Trailing whitespace

::: browser/devtools/inspector/test/browser_inspector_infobar_01.js
@@ +16,3 @@
>    let testData = [
>      {
> +      node: "#top",

We should remove xxx.node: and stick with xxx.id... there is no point having two identical attributes here.

::: browser/devtools/inspector/test/browser_inspector_infobar_02.js
@@ -29,5 @@
> -
> -  gBrowser.removeCurrentTab();
> -});
> -
> -function* checkInfoBarAboveTop(inspector) {

This is the only test we have that checks that the nodeinfobar is never displayed higher than or lower than the content area... did we really mean to remove it?

::: browser/devtools/inspector/test/browser_inspector_initialization.js
@@ +95,2 @@
>    try {
> +    is(inspector.markup._selectedContainer.node, nodeFront,

I know this is not new, but should we really be playing with the markup view's privates here?

::: browser/devtools/inspector/test/browser_inspector_invalidate.js
@@ +23,3 @@
>    div.style.width = "200px";
>  
> +  info("Polling the highlighter's size to verify that it changes too");

Is it not possible to use an event here? I realize that we have a limited environment inside a canvas frame but polling like this will produce oranges on a busy try server... it can be a few seconds from one line to the next in some circumstances.

::: browser/devtools/inspector/test/doc_frame_script.js
@@ +168,5 @@
> + */
> +addMessageListener("Test:SynthesizeMouse", function(msg) {
> +  let {node} = msg.objects;
> +  let {x, y, center, type} = msg.data;
> +  

NIT: Trailing whitespace
Attachment #8506997 - Flags: feedback?(mratcliffe)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #32)

Thanks for the feedback Mike. So I take it you do agree with the main approach of using a frame script to retrieve the highlighter actor instances, since you didn't mention that in your comments.

I have renamed/simplified/corrected a few things thanks to your feedback.

About the zoom and highlighter-out-of-content-area tests: these are gone because of the nature of the new highlighter. Find more info in my replies below:

> ::: browser/devtools/inspector/test/browser_inspector_highlighter-02.js
> @@ +25,3 @@
> >  
> >    info("Selecting the rotated DIV");
> > +  yield selectAndHighlightNode("#rotated-div", inspector);
> 
> This is the only test we have that checks if zoomed pages are correctly
> highlighted so we shouldn't remove that part of the test.
Filed follow-up bug 1084442 for taking care of zoom. 
The thing is, now that the highlighter is part of the document, it gets zoomed in/out with the content. So knowing how we want to deal with this and how to fix this test is going to take enough effort to be done in its own bug. That's why I decided to remove this part.

> ::: browser/devtools/inspector/test/browser_inspector_infobar_02.js
> @@ -29,5 @@
> > -
> > -  gBrowser.removeCurrentTab();
> > -});
> > -
> > -function* checkInfoBarAboveTop(inspector) {
> 
> This is the only test we have that checks that the nodeinfobar is never
> displayed higher than or lower than the content area... did we really mean
> to remove it?
The new highlighter is injected in the document's canvasFrame now, it just can *not* be displayed outside the content area now, so this whole logic isn't needed anymore, and the test either.

> ::: browser/devtools/inspector/test/browser_inspector_invalidate.js
> @@ +23,3 @@
> >    div.style.width = "200px";
> >  
> > +  info("Polling the highlighter's size to verify that it changes too");
> 
> Is it not possible to use an event here? I realize that we have a limited
> environment inside a canvas frame but polling like this will produce oranges
> on a busy try server... it can be a few seconds from one line to the next in
> some circumstances.
This doesn't have to do with the canvasframe environment, it's just that the test changes the content 'div.style.width = "200px";' and once this is done, there's no easy way to know when the highlighter will actually have updated to reflect the new size.
The only types of events we could have here are protocol.js events that the actor would send to the front to inform listeners that the highlighter geometry has changed, but since we update on paints, the amount of events we send would be far too much.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #32)
> ::: browser/devtools/inspector/test/browser_inspector_invalidate.js
> @@ +23,3 @@
> >    div.style.width = "200px";
> >  
> > +  info("Polling the highlighter's size to verify that it changes too");
> 
> Is it not possible to use an event here? I realize that we have a limited
> environment inside a canvas frame but polling like this will produce oranges
> on a busy try server... it can be a few seconds from one line to the next in
> some circumstances.

I could add an emitted event to the BoxModelHighlighter class which would actually not be sent via the protocol and would be here only for tests. Do you think this would be better?
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> I could add an emitted event to the BoxModelHighlighter class which would
> actually not be sent via the protocol and would be here only for tests. Do
> you think this would be better?

Yes, I think that would be best.
Blocking bug 1020244 hasn't landed yet, but all of its patches have been R+'d and there's only one test that fails on try, so it's that close to land now.

I think it makes sense to start getting reviews for the highlighter patches now. So here it is.

Mike, here's what this patch contains:

- Cleanup of the highlighter css files. There's only one css now.

- Changed the HighlighterActor so that it always uses the BoxModelHighlighter class now (except for XUL windows, where it still uses the simple outline).

- Logic to switch between the BoxModelHighlighter and SimpleOutlineHighlighter (and back) when navigating between HTML and XUL window types.

- Added a new CanvasFrameAnonymousContentHelper class that highlighters use. It wraps the new anonymous content API. Now, highlighters just have to provide a function that builds and returns the required markup. The CanvasFrameAnonymousContentHelper class uses this function whenever needed.

- CanvasFrameAnonymousContentHelper makes sure to re-insert the highlighter markup on page navigation. Indeed, the new API is at Document level, which means that the highlighter markup gets unbound and destroyed when the document is destroyed.

- Also introduced a new AutoRefreshHighlighter parent class that highlighters can extend from to be updated on mozAfterPaint. It's basically what XULBasedHighlighter used to be.

- Extensively refactored BoxModelHighlighter and CssTransformHighlighter to make use of the new API via CanvasFrameAnonymousContentHelper.

- Moved installHelperSheet into a global function. This used to be only used in SimpleOutlineHighlighter but is now the way highlighters CSS gets loaded into documents.

NOTE: Ideally, we'd need bug 1086532 to land before this patch so that we wouldn't have to load the CSS this way. Loading the CSS like that has several disadvantages but is the only way I found that works right now. See my XXXpbrosset comments in the code.
Attachment #8506992 - Attachment is obsolete: true
Attachment #8508702 - Flags: review?(mratcliffe)
Make sure not to update the highlighters too many times for nothing.
Attachment #8508703 - Flags: review?(mratcliffe)
Added an "updated" event to the highlighter so that tests can use it to know when the highlighter's position/size has been updated instead of having to poll like one of the tests used to do. This was reported by Mike previously.
I'd still like to have a second pair of eyes on the actorInstances/frame-script mechanism part, so leaving F? for Brian.
Attachment #8506997 - Attachment is obsolete: true
Attachment #8506997 - Flags: feedback?(bgrinstead)
Attachment #8508708 - Flags: feedback?(bgrinstead)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #38)
> Created attachment 8508708 [details] [diff] [review]
> bug985597-boxmodelhighlighter-in-canvasframe-tests v5.patch
> 
> Added an "updated" event to the highlighter so that tests can use it to know
> when the highlighter's position/size has been updated instead of having to
> poll like one of the tests used to do. This was reported by Mike previously.
> I'd still like to have a second pair of eyes on the
> actorInstances/frame-script mechanism part, so leaving F? for Brian.

Oh and, I need to explain what I did in the "tests" patch:

- refactored all browser/devtools/inspector/test/ tests to use the new add_task
- moved all highlighter-related tests to browser/devtools/inspector/test/
  - moved 3 highlighter tests that were in markupview
  - and moved all the highlighter tests that were in toolkit/server/test
- introduced a frame script that helps with testing the highlighter easily (by accessing the highlighter instance directly)
- I left the skip-if e10s in the inspector test browser.ini file because not all of them pass yet, but they're a lot closer now (the rest should be fixed in bug 1074836)
Comment on attachment 8508702 [details] [diff] [review]
bug985597-boxmodelhighlighter-in-canvasframe v10.patch

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

Just a few nits, awesome work!

::: browser/themes/shared/devtools/highlighter.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + XXXpbrosset For now this sheet is loaded as a user-agent style in highlighter.js

Don't know if you want to keep XXXpbrosset here.

::: toolkit/devtools/server/actors/highlighter.js
@@ +384,5 @@
>  let CustomHighlighterFront = protocol.FrontClass(CustomHighlighterActor, {});
>  
>  /**
> + * Every highlighters should insert their markup content into the document's
> + * canvasFrame anonymous content container (see Document.webidl).

We may be better using dom/webidl/Document.webidl here so that it is more obvious what we are talking about.

@@ +427,5 @@
> +  _insert: function() {
> +    // Re-insert the content node after page navigation only if the new page
> +    // isn't XUL.
> +    if (!isXUL(this.tabActor)) {
> +      // XXXpbrosset For now highlighter.css is injected in content as a ua

Mote sure if you want XXXpbrosset here
Attachment #8508702 - Flags: review?(mratcliffe) → review+
Comment on attachment 8508703 [details] [diff] [review]
bug985597-filter-paint-updates v1.patch

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +601,5 @@
> +  },
> +
> +  /**
> +   * Does this paint event actually contains painted rects for the content area.
> +   * XXXpbrosset for now we don't check that the painted rects are actually in

Not sure if you want XXXpbrosset here
Attachment #8508703 - Flags: review?(mratcliffe) → review+
Thanks Mike for the quick review.
I modified the comments as requested.
Attachment #8508702 - Attachment is obsolete: true
Attachment #8509288 - Flags: review+
Also updated the comments as requested. Thanks.
Attachment #8508703 - Attachment is obsolete: true
Attachment #8509291 - Flags: review+
Blocks: 1074836
Very minor update to the csstransform highlighter that removes an unecessary "hidden" attribute on the <g> tag.
Attachment #8509288 - Attachment is obsolete: true
Attachment #8509369 - Flags: review+
I think this requires a new review.
This is the same filter-paint patch as before except that I've completely remove the dependency on 'mozAfterPaint' in this patch:

I realized earlier today that it didn't work at all with e10s. That is to say that the highlighter wasn't updating on paint.
This is because paint events aren't forwarded by default to content, and we don't want to force this in the devtools. And with e10s, we don't have access to a browser window to listen to mozAftePaint.

Also, we've been contemplating for some time getting rid of this mozAfterPaint event updating mechanism because it's prone to bugs and doesn't let us filter out what's content paints from chrome paints.

Finally, the first version of this patch introduced a new '_hasMoved' function that checks if the highlighters needs updating when we're about to update.

So, with all this in mind, I decided to remove the mozAfterPaint completely and instead use a requestAnimationFrame loop.
When the highlighter is shown, just start looping, and check if the node has moved at every step of the way, only actually updating the highlighter if the node's geometry changed.

Anyway, we were using the mozAfterPaint listener as a looping mechanism already, so at least now, we have a real loop, and a way to update only when needed.
Attachment #8509291 - Attachment is obsolete: true
Attachment #8509414 - Flags: review?(mratcliffe)
Comment on attachment 8509414 [details] [diff] [review]
bug985597-filter-paint-updates v3.patch

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

This looks great... you know I never liked us using mozAfterPaint so I am more than happy with this approach.

r++++!
Attachment #8509414 - Flags: review?(mratcliffe) → review+
Change to highlighter.css to use the new chrome-only pseudo-class that landed with bug 1082899.
This simplifies the file and, more importantly, makes it impossible to impact web content (as the pseudo-class is chrome-only).

Mike, can you review this change? I'll fold it in with the first patch when reviewed. Just keeping them separate for now to simplify the review.
Attachment #8511024 - Flags: review?(mratcliffe)
Joe, the highlighter is changing a lot with this bug. The highlight command test used to access the browser XUL elements to get to the highlighter. This can't work anymore.
I've instead exposed the highlighter class instances at the module level to help testing.
This isn't e10s safe at the moment, but can easily be moved to a frame script when we make gcli tests e10s compatible.
Attachment #8511035 - Flags: review?(jwalker)
Small follow-up patch to fix a JS error that occurred when hovering a hidden node in the wbe console.
This is something that had been fixed in the past, and I must have broken it when working on the highlighter.
I will also fold it in the main patch when reviewed.
Attachment #8511040 - Flags: review?(mratcliffe)
Attachment #8511035 - Flags: review?(jwalker) → review+
Attachment #8511024 - Flags: review?(mratcliffe) → review+
Comment on attachment 8511040 [details] [diff] [review]
bug985597-dont-highlight-hidden-nodes v1.patch

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +1023,5 @@
>          Cu.isDeadWrapper(this.currentNode) ||
>          this.currentNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
>          !this.currentNode.ownerDocument ||
> +        !this.currentNode.ownerDocument.defaultView ||
> +        hasNoQuads) {

We used to do this but we removed it because people thought we were failing to highlight a node.

Even if a node has no quads it is nice to know where it is when hovering the node in the markup view where we should still show the guides over the node's position.
Attachment #8511040 - Flags: review?(mratcliffe)
Folded attachment 8511024 [details] [diff] [review] in this patch.
Attachment #8509369 - Attachment is obsolete: true
Attachment #8511024 - Attachment is obsolete: true
Attachment #8513483 - Flags: review+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #50)
> Comment on attachment 8511040 [details] [diff] [review]
> bug985597-dont-highlight-hidden-nodes v1.patch
> 
> Review of attachment 8511040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +1023,5 @@
> >          Cu.isDeadWrapper(this.currentNode) ||
> >          this.currentNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
> >          !this.currentNode.ownerDocument ||
> > +        !this.currentNode.ownerDocument.defaultView ||
> > +        hasNoQuads) {
> 
> We used to do this but we removed it because people thought we were failing
> to highlight a node.
> 
> Even if a node has no quads it is nice to know where it is when hovering the
> node in the markup view where we should still show the guides over the
> node's position.
Well this patch will only avoid trying to highlight nodes that aren't rendered at all in the render tree (display:none, <head>, etc ...). Nodes that have 0x0 dimensions, or that are out of the viewport are still rendered and *will* have quads, so those nodes will be highlighted, even if just guides are shown and the infobar shows 0x0 dimension.

So I still think this patch is correct and it helps get rid of a js error I had while running webconsole tests.
Comment on attachment 8511040 [details] [diff] [review]
bug985597-dont-highlight-hidden-nodes v1.patch

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #52)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #50)
> > Comment on attachment 8511040 [details] [diff] [review]
> > bug985597-dont-highlight-hidden-nodes v1.patch
> > 
> > Review of attachment 8511040 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/devtools/server/actors/highlighter.js
> > @@ +1023,5 @@
> > >          Cu.isDeadWrapper(this.currentNode) ||
> > >          this.currentNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
> > >          !this.currentNode.ownerDocument ||
> > > +        !this.currentNode.ownerDocument.defaultView ||
> > > +        hasNoQuads) {
> > 
> > We used to do this but we removed it because people thought we were failing
> > to highlight a node.
> > 
> > Even if a node has no quads it is nice to know where it is when hovering the
> > node in the markup view where we should still show the guides over the
> > node's position.
> Well this patch will only avoid trying to highlight nodes that aren't
> rendered at all in the render tree (display:none, <head>, etc ...). Nodes
> that have 0x0 dimensions, or that are out of the viewport are still rendered
> and *will* have quads, so those nodes will be highlighted, even if just
> guides are shown and the infobar shows 0x0 dimension.
> 
> So I still think this patch is correct and it helps get rid of a js error I
> had while running webconsole tests.

Okay, if it fixes errors then that is fine.
Attachment #8511040 - Flags: review+
Comment on attachment 8508708 [details] [diff] [review]
bug985597-boxmodelhighlighter-in-canvasframe-tests v5.patch

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

I've looked over doc_frame_script.js and some sample usage throughout the tests.  I don't have any major complaints about the way that the frame script is exposing the existing API for the canvas frame - I understand that the API itself is limited and this seems like a pretty direct mapping of it.  It's probably not worth trying to fake it and expose a more DOM-like version of that API just to make those few methods feel more comfortable, since that will just be frustrating when all other DOM functionality doesn't work.

::: browser/devtools/inspector/test/doc_frame_script.js
@@ +20,5 @@
> +let EventUtils = {};
> +loader.loadSubScript("chrome://marionette/content/EventUtils.js", EventUtils);
> +
> +/**
> + * Require the highlighter actor module, in the context of this frame script,

Do you mean "Requiring"?  Otherwise I'm not exactly sure what this sentence is saying

@@ +36,5 @@
> +  let {require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +  let {actorInstances} = require("devtools/server/actors/highlighter");
> +
> +  // Return the last actor created with the given type
> +  let arr = [...actorInstances];

Could just return:

[...actorInstances].reverse().find(actor=>actor.typeName === typeName)

@@ +111,5 @@
> + * SelectorHighlighter was not found.
> + */
> +addMessageListener("Test:GetSelectorHighlighterBoxNb", function() {
> +  let {_highlighter: h} = getCurrentHighlighterActor("customhighlighter");
> +  if (!h || !h._highlighters) {

Nit: I'd just use an if/else instead of an early return here since there is only one thing to do after the return anyway

@@ +140,5 @@
> +  h.currentNode.setAttribute(name, value);
> +});
> +
> +/**
> + * Get the element at the given x/y coordinates.

There is browser/devtools/shared/frame-script-utils.js that seems like it would be an appropriate place for these general testing functions: Test:ElementFromPoint, Test:GetAllAdjustedQuads, Test:SynthesizeMouse,Test:HasPseudoClassLock.

I'm not sure if that file is at the right path (maybe it should be in shared/test).  And it would complicate things with the executeInContent function if you had two separate files.  But I think it makes sense to keep a lot of this general stuff in one place for reuse across folders.

Would it be possible maybe to import the doc_frame_script file into shared/frame-script-utils.js instead?  Then all test folders could follow this pattern and all would only ever need to load frame-script-utils.js.

I wouldn't block this patch from landing because of this, I'm just trying to think of ways to avoid going in the direction of our many similar but different devtools/*/test/head.js files.  If you'd rather deal with this separately could you please file a follow up bug for sharing frame script functionality across tests?
Attachment #8508708 - Flags: feedback?(bgrinstead) → feedback+
Thanks for the feedback Brian.

(In reply to Brian Grinstead [:bgrins] from comment #54)
> Comment on attachment 8508708 [details] [diff] [review]
> bug985597-boxmodelhighlighter-in-canvasframe-tests v5.patch
> 
> Review of attachment 8508708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've looked over doc_frame_script.js and some sample usage throughout the
> tests.  I don't have any major complaints about the way that the frame
> script is exposing the existing API for the canvas frame - I understand that
> the API itself is limited and this seems like a pretty direct mapping of it.
> It's probably not worth trying to fake it and expose a more DOM-like version
> of that API just to make those few methods feel more comfortable, since that
> will just be frustrating when all other DOM functionality doesn't work.
> 
> ::: browser/devtools/inspector/test/doc_frame_script.js
> @@ +20,5 @@
> > +let EventUtils = {};
> > +loader.loadSubScript("chrome://marionette/content/EventUtils.js", EventUtils);
> > +
> > +/**
> > + * Require the highlighter actor module, in the context of this frame script,
> 
> Do you mean "Requiring"?  Otherwise I'm not exactly sure what this sentence
> is saying
I discovered that the way I was storing actor and highlighter instances on the module was causing leaks in a couple of debugger/webconsole tests because of the way these tests ended.
After investigating how to fix those tests, I decided to change the approach instead and remove the |actorInstances| code altogether in highlighter.js.
Now, instead, the test frame-script goes via the DebuggerServer's connection to retrieve the actors.
This way, there's no need to store anything, and the leaks are fixed.
It also means I rewrote that comment altogether, it should make a lot more sense now.

> @@ +36,5 @@
> > +  let {require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> > +  let {actorInstances} = require("devtools/server/actors/highlighter");
> > +
> > +  // Return the last actor created with the given type
> > +  let arr = [...actorInstances];
> 
> Could just return:
> 
> [...actorInstances].reverse().find(actor=>actor.typeName === typeName)
No need for this change anymore since actorInstances is gone.

> @@ +111,5 @@
> > + * SelectorHighlighter was not found.
> > + */
> > +addMessageListener("Test:GetSelectorHighlighterBoxNb", function() {
> > +  let {_highlighter: h} = getCurrentHighlighterActor("customhighlighter");
> > +  if (!h || !h._highlighters) {
> 
> Nit: I'd just use an if/else instead of an early return here since there is
> only one thing to do after the return anyway
Done.

> @@ +140,5 @@
> > +  h.currentNode.setAttribute(name, value);
> > +});
> > +
> > +/**
> > + * Get the element at the given x/y coordinates.
> 
> There is browser/devtools/shared/frame-script-utils.js that seems like it
> would be an appropriate place for these general testing functions:
> Test:ElementFromPoint, Test:GetAllAdjustedQuads,
> Test:SynthesizeMouse,Test:HasPseudoClassLock.
> 
> I'm not sure if that file is at the right path (maybe it should be in
> shared/test).  And it would complicate things with the executeInContent
> function if you had two separate files.  But I think it makes sense to keep
> a lot of this general stuff in one place for reuse across folders.
> 
> Would it be possible maybe to import the doc_frame_script file into
> shared/frame-script-utils.js instead?  Then all test folders could follow
> this pattern and all would only ever need to load frame-script-utils.js.
> 
> I wouldn't block this patch from landing because of this, I'm just trying to
> think of ways to avoid going in the direction of our many similar but
> different devtools/*/test/head.js files.  If you'd rather deal with this
> separately could you please file a follow up bug for sharing frame script
> functionality across tests?
Good point. As discussed on IRC yesterday with Jordan, there are indeed some message listeners we should put in common for all our tests.
I'm not too sure though that the common frame script should import all the other ones. Why not just make sure we put everything common in frame-script-utils.js (like Test:ElementFromPoint, Test:SynthesizeMouse, ...) and keep the rest in tool-specific framescripts?
Filed bug 1092097 for this.
v14 of the main highlighter patch.
I'm adding Mike's R+ here as the patch didn't change except for the fact that I've whitelisted highlighter.css in browser_parsable_css.js test as discussed with Gijs.
I'm going to flag him for review on this patch too.
Attachment #8513483 - Attachment is obsolete: true
Attachment #8515040 - Flags: review+
Comment on attachment 8515040 [details] [diff] [review]
bug985597-boxmodelhighlighter-in-canvasframe v14.patch

Gijs, could you take a look at the changes made to browser_parsable_css.js in this patch?
Attachment #8515040 - Flags: review?(gijskruitbosch+bugs)
v6 of the test patch, which now contains the GCLI test changes that Joe previously reviewed.

Mike, this is the biggest patch, but I had already F? you on it previously. Not much has changed since.

The biggest change is:

I've removed the actorInstances/highlighterInstances mechanism in highlighter.js. For info, this was in place so that tests could retrieve the highlighter and check that things were ok. I have changed this so that now the frame script goes via the DebuggerServer's connection instead, as this was causing leaks.
This is a better overall approach.

Other than this, the other change is that I added an event for when the highlighter changes position, to avoid having to poll in tests, according to your feedback.
Attachment #8508708 - Attachment is obsolete: true
Attachment #8511035 - Attachment is obsolete: true
Attachment #8515045 - Flags: review?(mratcliffe)
Comment on attachment 8515040 [details] [diff] [review]
bug985597-boxmodelhighlighter-in-canvasframe v14.patch

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

::: browser/base/content/test/general/browser_parsable_css.js
@@ +14,5 @@
>    {sourceName: /codemirror\.css/i}, /* CodeMirror is imported as-is, see bug 1004423 */
>    {sourceName: /web\/viewer\.css/i, errorMessage: /Unknown pseudo-class.*(fullscreen|selection)/i }, /* PDFjs is futureproofing its pseudoselectors, and those rules are dropped. */
>    {sourceName: /aboutaccounts\/(main|normalize)\.css/i}, /* Tracked in bug 1004428 */
> +  {sourceName: /loop\/.*sdk-content\/.*\.css$/i /* TokBox SDK assets, see bug 1032469 */},
> +  {sourceName: /highlighter\.css/i /* Highlighter CSS uses chrome-only pseudo-class, see bug 985597 */}

Presumably you can use an errorMessage regex for the pseudo class error you get here in addition to the filename? Something like:

errorMessage: /Unknown pseudo-class.*moz-native-anonymous/i

?

With that, r=me
Attachment #8515040 - Flags: review?(gijskruitbosch+bugs) → review+
Adapted the test as per Gijs' review comment.
Attachment #8515040 - Attachment is obsolete: true
Attachment #8516663 - Flags: review+
I noticed a few failing layoutview tests on try, so here's a patch for these tests to pass. It should also help make them more robust:

- it removes the need to actually test the highlighter's position, since this is already tested in the inspector tests
- it mocks the highlighter show/hide methods, to avoid interfering with the tests
- it makes sure all inspector panel are updated when a style change is made
- it also removes the asyncTest function since we can now use add_task in mochitests.
Attachment #8516665 - Flags: review?(mratcliffe)
Attachment #8515045 - Flags: review?(mratcliffe) → review+
Attachment #8516665 - Flags: review?(mratcliffe) → review+
Depends on: 1102269
Blocks: 1102273
Blocks: 1102269
No longer depends on: 1102269
Blocks: 1102276
Depends on: 1112465
Depends on: 1116714
Depends on: 1059312
Depends on: 1246087
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: