Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 1075490 - Fix devtools docking. #1829

Merged
merged 10 commits into from
Apr 22, 2015
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jan 9, 2015

Depends on platform change attached here (https://bug1075490.bugzilla.mozilla.org/attachment.cgi?id=8546311)

// panel documents in a content process, but for now platform implementation
// is buggy on linux so this is disabled.
// We end up using chrome iframe with forced message manager
// as fixing a swapFrameLoader seemd like a giant task (see
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: seemd -> seemed

@@ -150,6 +169,7 @@ exports["test Panel communication"] = test(function*(assert) {
yield closeToolbox();

assert.equal(panel.readyState, "destroyed", "panel is destroyed");
myTool.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems unrelated, if it is then please make a separate bug for this change.

@erikvold
Copy link
Contributor

We should have a test that it's not possible to have the local content (which passes validation) redirect to non-local content.

@erikvold
Copy link
Contributor

This is failing on travis:

7114 of 7118 tests passed.

JPM info There were test failures...
JPM info
The following tests failed:

JPM info ./test/test-dev-panel.test Panel API: timed out
./test/test-dev-panel.test Panel communication: timed out

JPM info ./test/test-dev-panel.test communication with debuggee: timed out
./test/test-dev-panel.test viewFor panel: timed out

@erikvold
Copy link
Contributor

I'm guessing that this.observe is undefined here? @Gozala does that make sense? and we should have a test that this works.

You are right! I had method before just changed it to arrow to match observe & have not run tests again. I'll update that.

Yes we do have tests for unload that were passing until I've changed method to arrow function.

@Gozala Which ones?

@@ -82,21 +85,21 @@ const observer = {
observerService.removeObserver(observer, topic);
}
else if (document === content.document) {
if (topic === "content-document-interactive") {
if (topic.endsWith("-document-interactive")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an unrelated change, please separate this change in to another bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an unrelated change, please separate this change in to another bug.

It is related because if content is chrome privileged topic will be `chrome-document-interactive"

@Gozala
Copy link
Contributor Author

Gozala commented Jan 27, 2015

This is failing on travis:

7114 of 7118 tests passed.

JPM info There were test failures...
JPM info
The following tests failed:

JPM info ./test/test-dev-panel.test Panel API: timed out
./test/test-dev-panel.test Panel communication: timed out

JPM info ./test/test-dev-panel.test communication with debuggee: timed out
./test/test-dev-panel.test viewFor panel: timed out

I've already pointed out that travis will fail .as it depend on platform change

@Gozala
Copy link
Contributor Author

Gozala commented Jan 27, 2015

We should have a test that it's not possible to have the local content (which passes validation) redirect to non-local content.

I'm afraid I can't make such guarantees, since we make this local content chrome privileged it can do whatever it pleased no matter how hard we try.

@Gozala
Copy link
Contributor Author

Gozala commented Jan 28, 2015

@erikvold I removed those myTool.destroy() calls now as well. But honestly I think you are not consistent with your requests, in some cases you ask me to add unrelated tests and in other cases you ask me to remove changes that ensure proper cleanup after test run.

Also now that related platform changes are in nightly travis is happy and passing tests!

@Gozala
Copy link
Contributor Author

Gozala commented Jan 28, 2015

Created separate pull for test cleanups as requested here #1847

@Gozala
Copy link
Contributor Author

Gozala commented Jan 28, 2015

Also here is a link to unload tests that came up in the discussion #1848

observerService.addObserver(observer, "content-page-hidden", false);
observerService.addObserver(observer, "chrome-document-interactive", false);
observerService.addObserver(observer, "chrome-document-loaded", false);
addEventListener("unload", observer, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you remove this listener at some point? Same question for observerService?
Or is this frame script global and want to dispatch events for all document living in the tab?

Gozala added a commit that referenced this pull request Apr 22, 2015
Bug 1075490 - Fix devtools docking. r=@ochameau
@Gozala Gozala merged commit 26182ea into mozilla:master Apr 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants