Closed
Bug 1245776
Opened 9 years ago
Closed 9 years ago
Convert devtools/client/inspector/shared/test to use shared-head.js
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
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).
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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/
Assignee | ||
Comment 4•9 years ago
|
||
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/
Assignee | ||
Comment 5•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8738447 -
Flags: review?(gl) → review+
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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/
Assignee | ||
Comment 10•9 years ago
|
||
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/
Assignee | ||
Comment 11•9 years ago
|
||
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/
Assignee | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
bugherder |
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: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•