Closed Bug 1124536 Opened 8 years ago Closed 8 years ago
I tweaked the above patch a bit, it's better to set the SourceActor's _contentType attribute in the constructor instead of changing it in a getter method. my doubt is: do we have to fix it in the SourceActor or in the actor which creates SourceActor instances?
Attachment #8552853 - Attachment is obsolete: true
This patch adds a new "browser_dbg_sources-pagemod.js" test case in the mochitest-devtools test suite.
Attachment #8554947 - Flags: review?(jlong)
by the way, this small change makes possible to use breakpoints on bookmarklets ;-)
Sorry, was at a conference last week so reviewing now!
Attachment #8554530 - Flags: review?(jlong) → review+
Comment on attachment 8554947 [details] [diff] [review] Bug1124536_DebuggerPageMod_fix_loadSourceError_testcase.patch Review of attachment 8554947 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks for the extensive test case. This looks good. Can you push to try with both these patches?
Attachment #8554947 - Flags: review?(jlong) → review+
I've just rebased the patch, fixed the nit and merged the testcase patch. Try pushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb2c5d8c3125
Backed out in https://hg.mozilla.org/integration/fx-team/rev/1821e8426f63 for xpcshell bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=1970090&repo=fx-team
I have a hard time reading if this is the patch's fault or not. Some of these errors look similar to something Eddy landed recently and had to back out. However, looking at the patch again, you probably want to remove the `console.warn`. I'm not sure if the console is available inside web workers. Either way, seeing as the errors are "console is not defined", that may fix the problem. It's not really a warning anyway. If the content type isn't detected, we get the content type from the HTTP header later when we fetch the source. We just don't do that for normal JS sources because we don't have to re-fetch them.
Sorry for that, I've removed the `console.warn` and pushed to try (`-u all` to ensure it doesn't fail in any testsuite): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7c473cd41a7
Attachment #8560909 - Attachment is obsolete: true
Attachment #8563168 - Attachment is obsolete: true
The above try seems okay (there are a couple of failures, but unrelated to the fix and test case added by this patch). I prefer this solution because it removes the addon-sdk pagemod module dependency and it works correctly on e10s right now (there are probably some issues between the pagemod module and e10s, but it's a different issue which it's probably better to be debugged separately, fixed inside the addon-sdk sources and tested using the addon-sdk test suite) How it looks to you?
Comment on attachment 8564185 [details] [diff] [review] Bug1124536_DebuggerPageMod_fix_loadSourceError.patch Review of attachment 8564185 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8564185 - Flags: review+
(In reply to Luca Greco from comment #17) > How it looks to you? Thanks for rewriting this, I like this version better. It's more standalone.
You need to log in before you can comment on or make changes to this bug.