Closed Bug 1245776 Opened 4 years ago Closed 3 years ago

Convert devtools/client/inspector/shared/test to use shared-head.js

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: pbro, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Tests in this directory should use common helpers from shared-head.js.
In fact, just like the tests in the devtools/client/inspector/rules/test directory, they should use the helpers from devtools/client/inspector/test/head.js (which itself imports shared-head.js).
Filter on CLIMBING SHOES
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Include inspector/test/head in shared/test/head and remove the redundant functions.
Comparing with markup/test/head and computed/test/head, some functions were also
duplicated and have been moved up to inspector/test/head or shared-head.

Logic for opening the inspector will be moved in part2.

Review commit: https://reviewboard.mozilla.org/r/44441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44441/
Attachment #8738447 - Flags: review?(gl)
Attachment #8738448 - Flags: review?(gl)
Attachment #8738449 - Flags: review?(gl)
shared/test/head had custom methods to open the Computed & Rule view.
They did not register any test actor and could be called several times
in the same test.

In order to reuse the openInspector method from inspector/test/head, the
tests have been modified.
We are moving from
openRuleView() -> do rule view tests -> openComputedView() -> do computed view tests
to
openInspector() -> select rule view -> do rule view tests -> select computed view -> do computed view tests

A bit more verbose for the tests themselves, but this will be useful when
migrating the shared tests to using the testActor.

Review commit: https://reviewboard.mozilla.org/r/44447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44447/
5 tests of the styleeditor suite are importing shared/test/head.
Now that this also includes shared-head.js, it conflicts with the existing
styleeditor head file.

For now, the import was moved to the head, so it's always imported.
The tests have been updated accordingly.

Should file a follow-up to split the inspector test suites head files in a
head.js + helpers.js (see commandline/test). It should be possible to
include helpers in other tests without impact on the test lifecycle.

Review commit: https://reviewboard.mozilla.org/r/44529/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44529/
For information, it looks like browser_styleinspector_csslogic-content-stylesheets.js is now hitting the issue from Bug 1250058 and leaks 2 docShells on e10s+debug.
Attachment #8738447 - Flags: review?(gl) → review+
Comment on attachment 8738447 [details]
MozReview Request: Bug 1245776 - part1: include inspector/test/head in shared/test/head;r=gl

https://reviewboard.mozilla.org/r/44441/#review43921
Comment on attachment 8738448 [details]
MozReview Request: Bug 1245776 - part2: remove custom openInspector methods;r=gl

https://reviewboard.mozilla.org/r/44447/#review43931

::: devtools/client/inspector/shared/test/head.js:91
(Diff revision 1)
>    });
>  };
>  
> -/**
> - * Open the toolbox, with the inspector tool visible.
> - *
> +function selectInspectorView(viewId, inspector) {
> +  inspector.sidebar.select(viewId);
> +  return inspector[viewId].view;

Sadly, this will only apply to rule and computed view since there is no .view component to the other inspector views. I wonder if we should keep the openRuleView and openComputedView methods making a call to selectInspectorView.

We also have methods for opening the rule view and computed view in their own head.js file. Should we not simply just move those out and share it everywhere?

I am okay with the changes made, but I think we should consider these issues I just mentioned.
Attachment #8738448 - Flags: review?(gl) → review+
Comment on attachment 8738449 [details]
MozReview Request: Bug 1245776 - part3: import shared/test/head in styleeditor/test/head;r=gl

https://reviewboard.mozilla.org/r/44529/#review43947

::: devtools/client/styleeditor/test/head.js:41
(Diff revision 1)
>    }, true);
>  
>    return def.promise;
> +};
> +
> +function* openRuleView() {

Similar to part 2, I would ultimately like to see us only have 1 shared openRuleView function in shared-head.js
Attachment #8738449 - Flags: review?(gl) → review+
Comment on attachment 8738447 [details]
MozReview Request: Bug 1245776 - part1: include inspector/test/head in shared/test/head;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44441/diff/1-2/
Comment on attachment 8738448 [details]
MozReview Request: Bug 1245776 - part2: remove custom openInspector methods;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44447/diff/1-2/
Comment on attachment 8738449 [details]
MozReview Request: Bug 1245776 - part3: import shared/test/head in styleeditor/test/head;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44529/diff/1-2/
https://reviewboard.mozilla.org/r/44529/#review43947

> Similar to part 2, I would ultimately like to see us only have 1 shared openRuleView function in shared-head.js

Meant to reply! 

Thanks for the suggestion.
Moved openRuleView and openComputedView from their respective head.js files to inspector/test/head.js (in part 2).
Here removed the local version of openRuleView.
https://reviewboard.mozilla.org/r/44447/#review43931

> Sadly, this will only apply to rule and computed view since there is no .view component to the other inspector views. I wonder if we should keep the openRuleView and openComputedView methods making a call to selectInspectorView.
> 
> We also have methods for opening the rule view and computed view in their own head.js file. Should we not simply just move those out and share it everywhere?
> 
> I am okay with the changes made, but I think we should consider these issues I just mentioned.

I moved openComputedView & openRuleView to inspector/test/head.js and am using them here as much as possible. I also added selectComputedView and selectRuleView methods. Here after opening the inspector a first time we really have to only switch the tab and not recreate a testactor. We could complexify the existing openRuleView and openComputedView in order to check if the inspector is already opened. But I think as a first step what we have here is good enough.
https://hg.mozilla.org/mozilla-central/rev/1a63948d29b2
https://hg.mozilla.org/mozilla-central/rev/cd879bc4cda1
https://hg.mozilla.org/mozilla-central/rev/a364e18f1d17
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.