Closed Bug 1466349 Opened 3 years ago Closed 3 years ago

Followup to 1456485, make Screenshots work on PDFs

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Version:
62 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr6061+ verified, firefox60 wontfix, firefox61+ verified, firefox62+ verified)

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ verified
firefox60 --- wontfix
firefox61 + verified
firefox62 + verified

People

(Reporter: zombie, Assigned: zombie)

References

Details

Attachments

(3 files)

Bug 1466349 part 1 - Enable passing addonData flags to embedded WE,
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1466349 part 2 - Pass addonData to embedded Screenshots WE, add permissions
59 bytes, text/x-review-board-request
aswan
: review+
ianbicking
: review+
Details
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns
59 bytes, text/x-review-board-request
aswan
: review+
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details 
There are a few issues, the main being that Screenshots is an embedded WE, which doesn't get passed proper addonData flags to enable privileged status and mozillaAddons permission.
Assignee: nobody → tomica
Blocks: 1456485
Status: NEW → ASSIGNED
Priority: -- → P1
Do we need to add a similar exception to manifest.json for reader mode too?
Attachment #8982843 - Flags: review?(aswan)
Attachment #8982844 - Flags: review?(jhirsch)
Attachment #8982844 - Flags: review?(aswan)
Attachment #8982919 - Flags: review?(aswan)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Do we need to add a similar exception to manifest.json for reader mode too?

Added.
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

I can confirm that the Try builds are working perfectly with both the PDF viewer and Reader Mode :)
Attachment #8982919 - Flags: feedback+
Comment on attachment 8982843 [details]
Bug 1466349 part 1 - Enable passing addonData flags to embedded WE,

https://reviewboard.mozilla.org/r/248706/#review255098

::: commit-message-8c926:1
(Diff revision 2)
> +Bug 1466349 part 1 - Enable passing addonData flags to embedded WE

This patch also fixes the creation of the matcher for origin permissions in child processes, please mention that in the commit message.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:136
(Diff revision 2)
> +   *   Additional data to pass to the Extension constructor.
>     *
>     * @returns {Promise<LegacyContextAPI>} A promise which resolve to the API exposed to the
>     *   legacy context.
>     */
> -  startup(reason) {
> +  startup(reason, addonData = null) {

nit: could just make the default here be {} and avoid the || below
Attachment #8982843 - Flags: review?(aswan) → review+
Comment on attachment 8982844 [details]
Bug 1466349 part 2 - Pass addonData to embedded Screenshots WE, add permissions

https://reviewboard.mozilla.org/r/248708/#review255102
Attachment #8982844 - Flags: review?(aswan) → review+
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

https://reviewboard.mozilla.org/r/248744/#review255106

::: commit-message-64fd3:1
(Diff revision 2)
> +Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

Please add an explanation for why we want to do such a think.
Attachment #8982919 - Flags: review?(aswan) → review+
Attachment #8982844 - Flags: review?(jhirsch) → review?(ianb)
Comment on attachment 8982844 [details]
Bug 1466349 part 2 - Pass addonData to embedded Screenshots WE, add permissions

https://reviewboard.mozilla.org/r/248708/#review255518

I've created a Screenshots bug for including this patch: https://github.com/mozilla-services/screenshots/issues/4528
Attachment #8982844 - Flags: review?(ianb) → review+
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b84f57938f8b
part 1 - Enable passing addonData flags to embedded WE, r=aswan
https://hg.mozilla.org/integration/autoland/rev/e04c7ccfd50e
part 2 - Pass addonData to embedded Screenshots WE, add permissions r=aswan,ianbicking
https://hg.mozilla.org/integration/autoland/rev/0bb149ce1d1a
part 3 - Ignore ignorePath for privileged about: MatchPatterns r=aswan
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

> Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1436482.

> [User impact if declined]:
Firefox Screenshots feature doesn't work on PDF pages.

> [Is this code covered by automated tests?]:
Partially.  Added tests to cover underlying WE support, not sure if the Screenshots team wants to add full integration testing.

> [Has the fix been verified in Nightly?]:
No, only manual testing done from the Try build by Ryan^.

> [Needs manual test from QE? If yes, steps to reproduce]: 
Yes, see bug 1456485.

> [List of other uplifts needed for the feature/fix]:
Bug 1456485.

> [Is the change risky?]:
Somewhat.

> [Why is the change risky/not risky?]:
It only enables privileged (mozilla-signed) extensions to access protected pages.

> [String changes made/needed]:
None.
Attachment #8982919 - Flags: approval-mozilla-beta?
(In reply to Tomislav Jovanovic :zombie from comment #19)
> > Approval Request Comment
> > [List of other uplifts needed for the feature/fix]:
> Bug 1456485.

Oops, forgot to note bug 1465544 as well.
Flags: qe-verify+
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

While I'd still like the SV folks to give this a more detailed look, I can at least confirm that everything appears to be working as expected with the latest Nightly build including these patches.

Approved for 61.0b12.
Attachment #8982919 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have verified this issue on the latest Nightly (62.0a1 Build ID: 20180606083531) build and the issue is no longer reproducible. Firefox Screenshots works now on PDF pages.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.
status-firefox62: fixed → verified
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

Verified on Beta, taking for ESR 60.1 as well.
Attachment #8982919 - Flags: approval-mozilla-esr60+
I have verified this issue on the latest Beta (61.0b14, Build ID: 20180614135649) and the issue is no longer reproducible.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.

The issue is still reproducible on Firefox ESR 60.0 because of bug 1467924 that should also be uplifted in ESR 60 (see bug 1467924 comment 10).
status-firefox61: fixed → verified
Product: Toolkit → WebExtensions
I have verified the issue on latest ESR (60.1.0, Build ID 20180621121604) and the issue is no longer reproducible. Tested on Windows 10 x64, Windows 7 x64, Mac 10.13 and Arch Linux 4.12.
Status: RESOLVED → VERIFIED
status-firefox-esr60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.