Closed Bug 997239 Opened 6 years ago Closed 6 years ago

Move console actor to a sdk module

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 3 obsolete files)

Web console is one of the actors that doesn't use sdk modules.
It faces the global scope sharing issue where dependencies of one file is randomly defined in another one.
It will also help switching to lazy loading actor, only implemented for actors defined as modules (bug 988237).
And it is also a dependency of bug 859372 work.
Priority: -- → P3
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a268f75f972d

Quite straightforward.

The only thing that questioned me is the hasNativeConsole thing,
I remember that I already did that (copy paste it) and I got r- from Panos.
As I don't really know what is the best option, I kept it as-is.
But we can't make webbrowser depend on webconsole
as it would defeat lazy loading of actors by forcing to load
the console even if you don't open the console panel.
(as hasNativeConsole is called as soon as you attach to a tab)
Attachment #8407631 - Flags: review?(mihai.sucan)
Attached patch patch (obsolete) — Splinter Review
Previous try run failed because I was using LongStringActor
from string.js instead of using the one from script.js...
Attachment #8407631 - Attachment is obsolete: true
Attachment #8407631 - Flags: review?(mihai.sucan)
Assignee: nobody → poirot.alex
Comment on attachment 8408193 [details] [diff] [review]
patch

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

/me likes!
Attachment #8408193 - Flags: review?(mihai.sucan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a6a7df40f00a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Depends on: 998898
Depends on: 998912
backed this out from m-c (and also in the next merge from the other integrations trees) too, for request from Alexandre, since this seems has caused regressions
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/99d367b1adb2

The patch broke on-device debugging (bug 998898) and browser toolbox (bug 998912).
I prefer to back it out and take some time to reland a patch that doesn't break stuff.
Target Milestone: Firefox 31 → ---
Attached patch patch (obsolete) — Splinter Review
This time I tested on device and the browser toolbox.
It appears that I got tricked by the "multiple main.js" for the browser toolbox breakage.
We shouldn't pull DebuggerServer from dbg-server.jsm, which always retrieve the shared one for tabs.
Instead we can pull it from require("devtools/server/main"), which will eventually return
the expected one in browser toolbox scenario.
I also broke b2g in previous patch. That time it was more obvious.
It was because of `devtools` symbols, previously defined in the "magic shared global scope"
by webconsole.js and used by webbrowser.js...

In this patch I'm only documenting the "magic shared global scope" for webbrowser,
as this isn't the goal of this bug to address webbrowser issues.
Eddy will most likely address that in bug 859372.
Attachment #8408193 - Attachment is obsolete: true
Comment on attachment 8410371 [details] [diff] [review]
patch

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

This looks good to me.  I've tested the browser toolbox case here too, seems fine now.

May need to rebase though, for me it conflicts with your patch from bug 997239.
Attachment #8410371 - Flags: review?(jryans) → review+
Attached patch patchSplinter Review
rebased.
Attachment #8410371 - Attachment is obsolete: true
Attachment #8411176 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ecdb94250102
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.