Closed Bug 1103993 Opened 5 years ago Closed 5 years ago

Only refresh the rule-view and computed-view when corresponding panels are active

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
Firefox 37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(4 files, 9 obsolete files)

91.30 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
5.20 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
12.22 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
37.91 KB, patch
pbro
: review+
Details | Diff | Splinter Review
This used to be the case about a year ago. I don't exactly remember the reason for removing this behavior, but the idea behind putting it back in place is to avoid unnecessary protocol traffic and therefore make the UI more responsive.

When selecting a new in the inspector, both the rule-view and the computed-view issue requests to the styles actor to get the right rules and properties.
This takes time, especially when there are lots of rules and properties (one example is gaia with the gaia-theme.css which has many css variables in the :root selector), and when debugging a remote device over USB.

The logic to only refresh a panel when it is visible is pretty simple and can easily be added back in the code.
Assignee: nobody → pbrosset
Blocks: 1098435
Status: NEW → ASSIGNED
part 1 - Just renames the highlight method, since this is confusing with the actual highlighter.
Attachment #8527775 - Flags: review?(bgrinstead)
part 2 - Some code cleanup only. Renaming functions mostly, to make it more obvious what they do.
Attachment #8527777 - Flags: review?(bgrinstead)
part 3 - This is the actual patch. That's where I changed the style-inspector so that the rule-view and computed-view would only refresh when their active (and when they are being switched to).
I needed to remove one test that was obsolete with this change, I added a new one instead.
And I had to correct a few tests.
Attachment #8527779 - Flags: review?(bgrinstead)
Comment on attachment 8527775 [details] [diff] [review]
bug1103993-1-rename-styleinspector-highlight.patch

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

::: browser/devtools/styleinspector/rule-view.js
@@ +1449,5 @@
>      let refreshOnPrefs = [PREF_UA_STYLES, PREF_DEFAULT_COLOR_UNIT];
>      if (refreshOnPrefs.indexOf(pref) > -1) {
>        let element = this._viewedElement;
>        this._viewedElement = null;
> +      this.selectNode(element);

Should be this.selectElement?
Attachment #8527775 - Flags: review?(bgrinstead) → review+
part 4 - And more cleanup! This time, the tests are being cleaned up. But just a little. Just removing the custom asyncTest implementation since we now have add_task. Less code and better error reporting.
Attachment #8527781 - Flags: review?(bgrinstead)
Comment on attachment 8527777 [details] [diff] [review]
bug1103993-2-styleinspector-code-format.patch

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

This diff is kind of hard to follow, but it looks like these are simple enough refactors

::: browser/devtools/styleinspector/style-inspector.js
@@ +10,2 @@
>  Cu.import("resource://gre/modules/Services.jsm");
> +const { PREF_ORIG_SOURCES } = require("devtools/styleeditor/utils");

Nit: get rid of spaces around brackets
Attachment #8527777 - Flags: review?(bgrinstead) → review+
Comment on attachment 8527779 [details] [diff] [review]
bug1103993-3-only-refresh-styleinspector-if-active.patch

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

Overall these changes look good - just have a question about the change in styles.js

::: browser/devtools/styleinspector/test/browser_styleinspector_tooltip-multiple-background-images.js
@@ +41,5 @@
>  
>  function* testComputedViewUrls() {
>    info("Testing tooltips in the computed view");
>  
> +  let { view, inspector } = yield openComputedView();

Nit: use consistent whitespace with brackets in rest of file (no space)

::: toolkit/devtools/server/actors/styles.js
@@ +307,5 @@
>     *   `matchedSeletors`: Include an array of specific selectors that
>     *     caused this rule to match its node.
>     */
>    getApplied: method(function(node, options) {
> +    this.cssLogic.highlight(node.rawNode);

Why does this now need to be called?  It seems unrelated to the rest of this patch.
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Comment on attachment 8527775 [details] [diff] [review]
> bug1103993-1-rename-styleinspector-highlight.patch
> 
> Review of attachment 8527775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1449,5 @@
> >      let refreshOnPrefs = [PREF_UA_STYLES, PREF_DEFAULT_COLOR_UNIT];
> >      if (refreshOnPrefs.indexOf(pref) > -1) {
> >        let element = this._viewedElement;
> >        this._viewedElement = null;
> > +      this.selectNode(element);
> 
> Should be this.selectElement?
Yes you're right, I'll correct this. This shows that we don't have a single test for this code path because all tests passed locally with this change.
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8527779 [details] [diff] [review]
> bug1103993-3-only-refresh-styleinspector-if-active.patch
> 
> Review of attachment 8527779 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/devtools/server/actors/styles.js
> @@ +307,5 @@
> >     *   `matchedSeletors`: Include an array of specific selectors that
> >     *     caused this rule to match its node.
> >     */
> >    getApplied: method(function(node, options) {
> > +    this.cssLogic.highlight(node.rawNode);
> 
> Why does this now need to be called?  It seems unrelated to the rest of this
> patch.
Without this change, there are dead object exceptions on page navigation. STR:
- open the rule-view
- switch to the computed-view
- go back to the rule-view
- reload the page
==> The rule-view is now blank with exceptions in the logs.
I don't understand why this line wasn't here before, it should really have been in getApplied all along. This makes sure cssLogic returns the information from the right node.
This probably used to work before just because it was done in getComputed.
Now that the 2 panels are initialized independently, we need to configure cssLogic with the current node in both getApplied and getComputed.
Comment on attachment 8527779 [details] [diff] [review]
bug1103993-3-only-refresh-styleinspector-if-active.patch

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

Looks good
Attachment #8527779 - Flags: review?(bgrinstead) → review+
Attachment #8527781 - Flags: review?(bgrinstead) → review+
Fixed method name typo.
Attachment #8527775 - Attachment is obsolete: true
Attachment #8527857 - Flags: review+
Fixes as per review comments.
Attachment #8527777 - Attachment is obsolete: true
Attachment #8527859 - Flags: review+
Fixes as per review comments.
Attachment #8527779 - Attachment is obsolete: true
Attachment #8527860 - Flags: review+
There is a backward compatibility issue with this fix:

the client-side changes I have made (only refreshing a panel when it is visible) depend on a server-side fix too (namely, calling cssLogic.highlight(node) is now required when getting rules for the rule-view).

Before my client-side changes, this wasn't required because we were refreshing both the rule-view and computed-view at the same time, every time we selected a new node.

This isn't the case anymore, and so to avoid deadobject exceptions when navigating to a new page without switching to the computed-view, we must call cssLogic.highlight(node) when getting rules for the rule-view too.

So with this fix in, if I connect to an older firefox via the remote protocol, because cssLogic.highlight(node) is not present on this older version, I will run into the following bug:
- open the inspector, on the rule-view
- select a node
- reload the page
--> rule view is blank
- now switch to the computed view again (to force a cssLogic.highlight(node))
- select another node
--> the rule view is back to normal.

This will not happen when debugging apps though, since there is no reload or page navigation involved there.
So, maybe we can live with this for a short while?
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #15)
> There is a backward compatibility issue with this fix:
> 
> the client-side changes I have made (only refreshing a panel when it is
> visible) depend on a server-side fix too (namely, calling
> cssLogic.highlight(node) is now required when getting rules for the
> rule-view).
> 
> Before my client-side changes, this wasn't required because we were
> refreshing both the rule-view and computed-view at the same time, every time
> we selected a new node.
> 
> This isn't the case anymore, and so to avoid deadobject exceptions when
> navigating to a new page without switching to the computed-view, we must
> call cssLogic.highlight(node) when getting rules for the rule-view too.
> 
> So with this fix in, if I connect to an older firefox via the remote
> protocol, because cssLogic.highlight(node) is not present on this older
> version, I will run into the following bug:
> - open the inspector, on the rule-view
> - select a node
> - reload the page
> --> rule view is blank
> - now switch to the computed view again (to force a cssLogic.highlight(node))
> - select another node
> --> the rule view is back to normal.
> 
> This will not happen when debugging apps though, since there is no reload or
> page navigation involved there.
> So, maybe we can live with this for a short while?
As discussed with Alex, this is a more important issue than I thought because it also occurs when reloading an app with the webide.
My plan right now to fix this AND keep some of the performance benefits is to detect whether or not the server has the call to cssLogic.highlight(node) when getting rules (a trait or anything else that works), and if not, call a method on the server that actually does that cssLogic.highlight(node) call. I think calling pageStyle.getLayout() would be best in terms of perf.
Attached patch bug1103993-3.1-test-fixes.patch (obsolete) — Splinter Review
As a follow-up to patch 3, this patch fixes various test failures.
Attachment #8528298 - Flags: review?(bgrinstead)
As a follow-up to patch 3, and related to comment 15 and comment 16: this patch handles the backward compatibility with older servers.
Attachment #8528300 - Flags: review?(poirot.alex)
Attachment #8528298 - Flags: review?(bgrinstead) → review+
Comment on attachment 8528300 [details] [diff] [review]
bug1103993-3.2-backward-compatibility.patch

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

I don't know what is the performances impact of calling getLayout on old devices,
but code looks good.

::: toolkit/devtools/server/actors/styles.js
@@ +717,5 @@
> +    if (detail === "actorid") {
> +      this.actorID = form;
> +      return;
> +    }
> +    this.actorID = form.actor;

protocol.js seems to already set this.actorID = form.actor;
I'm even wondering if that also does the this.actorID = form somewhere...

@@ +743,5 @@
> +    // means a call to cssLogic.highlight is required before trying to access
> +    // the applied rules. Issue a request to getLayout if this is the case.
> +    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1103993#c16.
> +    if (!this._form.traits || !this._form.traits.getAppliedCreatesStyleCache) {
> +      yield this.getLayout(node);

Given that getLayout ends up calling CssLogic.getComputedStyle() isn't it going to be twice as slow because of that? (if the root cause of slowness if this method?)
Attachment #8528300 - Flags: review?(poirot.alex) → review+
The call to getLayout takes between 0.6 and 1s on my unagi...
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> The call to getLayout takes between 0.6 and 1s on my unagi...
Yes, it's not going to be as fast as not calling anything, but this is only for the case where the client-side has the fix but not the server-side. And it still should be similar performance (if not faster) than calling both getApplied and getComputed (which it does today).
Pushed part 1 and 2 to fx-team (these were just minor code re-formatting + method renaming):

remote:   https://hg.mozilla.org/integration/fx-team/rev/906e87fee4a6
remote:   https://hg.mozilla.org/integration/fx-team/rev/bcb6a96a383b
Keywords: leave-open
Attachment #8527857 - Flags: checkin+
Attachment #8527859 - Flags: checkin+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21)
> (In reply to Alexandre Poirot [:ochameau] from comment #20)
> > The call to getLayout takes between 0.6 and 1s on my unagi...
> Yes, it's not going to be as fast as not calling anything, but this is only
> for the case where the client-side has the fix but not the server-side. And
> it still should be similar performance (if not faster) than calling both
> getApplied and getComputed (which it does today).
I just realized I entered this comment without actually seeing you had R+'d my patch before with some comments.
I'll take a closer look at other alternatives than using getLayout.
Thanks for the review Alex.
Green-looking try build with all the patches so far: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=211c2855b819
I'm yet to correct the backwards-compatibility patch as per Alex's comments.
And I need to attach another test-fixes patch too.
(In reply to Alexandre Poirot [:ochameau] from comment #19)
> @@ +743,5 @@
> > +    // means a call to cssLogic.highlight is required before trying to access
> > +    // the applied rules. Issue a request to getLayout if this is the case.
> > +    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1103993#c16.
> > +    if (!this._form.traits || !this._form.traits.getAppliedCreatesStyleCache) {
> > +      yield this.getLayout(node);
> 
> Given that getLayout ends up calling CssLogic.getComputedStyle() isn't it
> going to be twice as slow because of that? (if the root cause of slowness if
> this method?)
No the slowness isn't caused by CssLogic.getComputedStyle(), this only returns what win.getComputedStyle(node) returns. As described in https://bugzilla.mozilla.org/show_bug.cgi?id=1098435#c15 the slowness is partly due to CssLogic.hasMatchedSelectors.

I have timed this change on my Peak, calling getLayout only adds about 15ms for me.
The other option I have instead is calling getMatchedSelectors but that takes around 350ms instead.
So I believe this is the best compromise.
Carrying R+ over for this minor patch update (removed this.actorID = form.actor as per feedback).
Attachment #8528300 - Attachment is obsolete: true
Attachment #8529064 - Flags: review+
One more patch. I believe this is the last required.
This one fixes a bunch of unhandled rejected promises and adds a new inspector test that ensures switching to the inspector and immediately closing the toolbox doesn't not fail.
Attachment #8529065 - Flags: review?(bgrinstead)
Comment on attachment 8529065 [details] [diff] [review]
bug1103993-3.11-more-test-failures.patch

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

Looks good and gets rid of logspam in the test.  But I'm seeing the test pass even without the patch applied.  Not sure exactly why this is, but we should make sure it fails without the changes so we can be sure they don't break later.  Also, please add a commit message.

::: browser/devtools/inspector/test/browser_inspector_destroy-before-ready.js
@@ +16,5 @@
> +  yield addTab("data:text/html;charset=utf-8,test inspector destroy");
> +
> +  info("Open the toolbox on the debugger panel");
> +  let target = TargetFactory.forTab(gBrowser.selectedTab);
> +  let toolbox = yield gDevTools.showToolbox(target, "debugger");

this should be "jsdebugger" (otherwise it will just end up opening on whatever was last used since there's no tab called "debugger")

::: browser/devtools/styleinspector/computed-view.js
@@ +588,5 @@
>      CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames,
>        mozProps.sort());
>  
> +    this._createPropertyViews().then(null, () => {
> +      console.warn("The creation of property views was cancelled. Most " +

I'm thinking we should do the same checks you do in markup-view / inspector-panel here so we can get a specific error or warning depending on if it's been destroyed.  That could help if someone is seeing this message and needs to figure out what is going on.

I know there isn't a _destroying object here, but you could probably check !this.styleInspector since that's set in the constructor and nulled out in the destroy function.
Attachment #8529065 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #28)
> Comment on attachment 8529065 [details] [diff] [review]
> bug1103993-3.11-more-test-failures.patch
> 
> Review of attachment 8529065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good and gets rid of logspam in the test.  But I'm seeing the test
> pass even without the patch applied.  Not sure exactly why this is, but we
> should make sure it fails without the changes so we can be sure they don't
> break later.
Really? That's weird, it does fail for me. If I just add the new test but do not apply this patch, I see the following failure: http://pastebin.mozilla.org/7584078

> Also, please add a commit message.
I'm going to fold all 3.x patches in patch 3. I only kept them separate for now to ease reviews. I'll make sure the final patch gets a proper commit message.

> ::: browser/devtools/inspector/test/browser_inspector_destroy-before-ready.js
> @@ +16,5 @@
> > +  yield addTab("data:text/html;charset=utf-8,test inspector destroy");
> > +
> > +  info("Open the toolbox on the debugger panel");
> > +  let target = TargetFactory.forTab(gBrowser.selectedTab);
> > +  let toolbox = yield gDevTools.showToolbox(target, "debugger");
> 
> this should be "jsdebugger" (otherwise it will just end up opening on
> whatever was last used since there's no tab called "debugger")
Good catch, thanks. Changing it now.

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +588,5 @@
> >      CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames,
> >        mozProps.sort());
> >  
> > +    this._createPropertyViews().then(null, () => {
> > +      console.warn("The creation of property views was cancelled. Most " +
> 
> I'm thinking we should do the same checks you do in markup-view /
> inspector-panel here so we can get a specific error or warning depending on
> if it's been destroyed.  That could help if someone is seeing this message
> and needs to figure out what is going on.
> 
> I know there isn't a _destroying object here, but you could probably check
> !this.styleInspector since that's set in the constructor and nulled out in
> the destroy function.
Yep, good idea, will change this.
Attachment #8529065 - Attachment is obsolete: true
Attachment #8529722 - Flags: review?(bgrinstead)
Pending try build for all patches in this bug: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9f6401f2987
Comment on attachment 8529722 [details] [diff] [review]
bug1103993-3.11-more-test-failures v2.patch

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

I'm happy with the changes overall, but I am still not having a failure in the test locally without the other patch changes applied.  I went ahead and pushed to try just to see (in theory this should turn up orange: https://tbpl.mozilla.org/?tree=Try&rev=8b95d56cf86b)
Comment on attachment 8529722 [details] [diff] [review]
bug1103993-3.11-more-test-failures v2.patch

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

Looks like the test didn't fail on try without the rest of the changes, so let's wait until we can confirm it is failing without the other changes before landing
Attachment #8529722 - Flags: review?(bgrinstead) → review-
(In reply to Brian Grinstead [:bgrins] from comment #34)
> Comment on attachment 8529722 [details] [diff] [review]
> bug1103993-3.11-more-test-failures v2.patch
> 
> Review of attachment 8529722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like the test didn't fail on try without the rest of the changes, so
> let's wait until we can confirm it is failing without the other changes
> before landing
Oh, I know what's going on, this new test I added only fails if attachment 8527860 [details] [diff] [review] is applied first.
Brian, could you run again that test alone (without the rest of the 3.11 patch), but first making sure patch 3 is applied?
Flags: needinfo?(bgrinstead)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #36)
> Brian, could you run again that test alone (without the rest of the 3.11
> patch), but first making sure patch 3 is applied?

Yeah, it's failing for me now so I think that patch is good to go
Flags: needinfo?(bgrinstead)
Attachment #8529722 - Flags: review- → review+
I'm not sure that this is going to fix the intermittent in Bug 1103953, but it will at least help when debugging it since it will clean up some of the log spam
Blocks: 1103953
Thanks Brian.
Folded all 3.x patches into 1.

Here's one more try push for good measure:https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5361f6eb39f8
Will land this in the morning.
Attachment #8527860 - Attachment is obsolete: true
Attachment #8528298 - Attachment is obsolete: true
Attachment #8529064 - Attachment is obsolete: true
Attachment #8529722 - Attachment is obsolete: true
Attachment #8533945 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e0bcde09cbeb
https://hg.mozilla.org/mozilla-central/rev/1ee8cdb3f47c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
This fixed bug 1103953, which also exists on Aurora. Any chance we can safely backport these patches?
Flags: needinfo?(pbrosset)
Out of the 4 patches: 
- the 2 first ones are already in Aurora
- the 3rd one (which is the one that actually contains the fix) applies cleanly on Aurora
- the 4th one doesn't apply, but we really don't care about it because it's only minor test syntax refactorings.
Flags: needinfo?(pbrosset)
Comment on attachment 8533945 [details] [diff] [review]
bug1103993-3-only-refresh-styleinspector-if-active.patch

Approval Request Comment
[Feature/regressing bug #]: This patch introduced lazy-refresh of the inspector sidebar panels, which ended up fixing the intermittent failing test described in bug 1103953.
[User impact if declined]: If declined, browser_inspector_iframe-navigation.js will keep on failing intermittently on Aurora.
[Describe test coverage new/current, TBPL]: The changes in this patch are tested by many automated browser mochitests in browser/devtools/inspector and browser/devtools/styleinspector.
[Risks and why]: This fix has been in Nightly for over a week with no reports of it breaking anything. It landed in Nightly not a long time after the uplift date to Aurora, so I think the risk of uplifting it is minimal.
[String/UUID change made/needed]: none.
Attachment #8533945 - Flags: approval-mozilla-aurora?
Attachment #8533945 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.