Need to shim protocol.js move for add-ons

RESOLVED FIXED in Firefox 49

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jryans, Assigned: jfong)

Tracking

({addon-compat, regression})

Trunk
Firefox 49
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox48 unaffected, firefox49+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
In bug 1270173, protocol.js was moved to shared.

However, there are also add-ons that load this module to define their own actors, including the WebSocket Monitor[1] that we suggest people use on hacks.m.o.

We should create a shim module so that is can still be loaded with the old module ID as well as the new one.

[1]: https://addons.mozilla.org/en-US/firefox/addon/websocket-monitor/
Whiteboard: [devtools-html] [triage]
Reporter

Comment 1

3 years ago
:ednapiranha, can you take a look at this?  Creating a small shim that lives in /devtools/server/shims and loads the module for the new ID should do it.
Flags: needinfo?(jfong)
Reporter

Comment 2

3 years ago
[Tracking Requested - why for this release]: Let's make sure we land something here in 49 to maintain add-on compat.
Keywords: addon-compat
Tracking for 49 as add on compat is important.
Assignee

Updated

3 years ago
Assignee: nobody → jfong
Flags: needinfo?(jfong)
Reporter

Updated

3 years ago
Priority: -- → P1
Blocks: 1263289
Priority: P1 → P2
Assignee

Comment 4

3 years ago
Posted patch Bug1271399.patch (obsolete) — Splinter Review
So I think I have most of the setup, but existing add-ons still aren't able to detect the path. I did notice that this path (EXTRA_JS_MODULES.devtools.server) points to resource://gre/modules/devtools/server/protocol.js rather than resource://devtools/server/protocol.js. 

Any idea what I might be missing here to make it point to the correct location?
Attachment #8750879 - Flags: feedback?(jryans)
Reporter

Comment 5

3 years ago
Comment on attachment 8750879 [details] [diff] [review]
Bug1271399.patch

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

::: devtools/server/shims/moz.build
@@ +15,5 @@
>  EXTRA_JS_MODULES.devtools += [
>      'dbg-server.jsm',
>  ]
> +
> +# Extra compatibility layer for transitional URLs used for part of 44 cycle

Update this comment to note that protocol.js was moved in 49

@@ +16,5 @@
>      'dbg-server.jsm',
>  ]
> +
> +# Extra compatibility layer for transitional URLs used for part of 44 cycle
> +EXTRA_JS_MODULES.devtools.server += [

Sorry, I gave you some bad info here! :(

Here's what should do the trick:

FINAL_TARGET_FILES.chrome.devtools.modules.devtools.server += [ ...

Now with more dots!

::: devtools/server/shims/protocol.js
@@ +33,5 @@
> +];
> +
> +const { require } =
> +  Cu.import("resource://devtools/shared/Loader.jsm", {});
> +const module = require("devtools/shared/protocol");

Since it's a CommonJS module, I think you can do:

module.exports = require("devtools/shared/protocol");

to export everything, and then you shouldn't need `EXPORTED_SYMBOLS` for this case.
Attachment #8750879 - Flags: feedback?(jryans)
Assignee

Comment 6

3 years ago
Added your changes, but still having path issues.
Attachment #8750879 - Attachment is obsolete: true
Attachment #8750914 - Flags: feedback?(jryans)
Reporter

Comment 7

3 years ago
Comment on attachment 8750914 [details] [diff] [review]
Bug1271399.patch

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

Looks like the DIST_SUBDIR in the moz.build is messing this up.  I'll attach my own attempt for you to check over.
Attachment #8750914 - Flags: feedback?(jryans)
Reporter

Comment 8

3 years ago
Attachment #8750927 - Flags: review?(jfong)
Reporter

Comment 9

3 years ago
The existing files are moved into a new directory (the diff view is bad at showing this, can check the patch file directly at the attachment link).
Comment on attachment 8750927 [details] [diff] [review]
Shim for protocol.js

Yep this works for me!
Attachment #8750927 - Flags: review?(jfong) → review+
Status: NEW → ASSIGNED
Flags: qe-verify-
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe3494949571
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Iteration: --- → 49.2 - May 23
Priority: P2 → P1

Updated

3 years ago
Blocks: 1277706

Updated

3 years ago
No longer blocks: 1263289
Blocks: 1263289
Version: unspecified → Trunk
Whiteboard: [devtools-html]

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.