Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ID in webext manifest #404

Merged
merged 1 commit into from Jan 25, 2018
Merged

Conversation

lmorchard
Copy link
Collaborator

Looks like the ID for the WebExtension in the manifest.json was left
off, which means the extension was getting a generated ID.

That ID didn't end in @mozilla.com, which means that the context-fill
feature
feature wasn't enabled.

This also would have caused other problems.

Fixes #401.

Looks like the ID for the WebExtension in the manifest.json was left
off, which means the extension was getting a generated ID.

That ID didn't end in @mozilla.com, which means that the [context-fill
feature][context-fill] feature wasn't enabled.

This also would have caused other problems.

[context-fill]: https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGContextPaint.cpp#69

Fixes bwinton#401.
'gecko': Object.assign({
'id': packageMeta.id
}, packageMeta.webextensionManifest.applications.gecko)
}
}, packageMeta.webextensionManifest);
Copy link
Collaborator Author

@lmorchard lmorchard Jan 25, 2018

Choose a reason for hiding this comment

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

Turns out that the change I made earlier that changed the order of merging these objects just ended up clobbering the applications.gecko.id assignment in the Object.merge call up there. So, simplifying things by just making a straightforward assignment

Copy link
Owner

Choose a reason for hiding this comment

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

Could we remove the id from webExtensionManifest.applications.gecko instead, if we know we want the packageMeta one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Problem is that there isn't an id in there already. The only property in there is update_url, and that was clobbering the merged object

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, cause they don't get merged… Dang. Okay, r=me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, Object.assign is a dumb flat merge ugh

'gecko': Object.assign({
'id': packageMeta.id
}, packageMeta.webextensionManifest.applications.gecko)
}
}, packageMeta.webextensionManifest);
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, cause they don't get merged… Dang. Okay, r=me.

@lmorchard lmorchard merged commit 7270613 into bwinton:master Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants