6

1664444 - Switch Screenshots from using drawWindow to captureTab, to work with F...

 3 years ago
source link: https://bugzilla.mozilla.org/show_bug.cgi?id=1664444
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client
Closed Bug 1664444 Opened 3 months ago Closed 5 days ago

Switch Screenshots from using drawWindow to captureTab, to work with Fission iframes

Categories

(Firefox :: Screenshots, defect, P1)

Tracking

(bug RESOLVED as FIXED)

RESOLVED FIXED

86 Branch

Tracking Status firefox86 --- fixed

People

(Reporter: zombie, Assigned: emalysz)

References

(Blocks 3 open bugs, Regressed 1 open bug)

Details

The canvas drawWindow method is sync, and thus can't be Fission-compatible for OOP iframes.

In bug 1636508, I introduced additional arguments to the tabs.captureTab method to specify an area of the document, and the scale (devicePixelRatio). They're not documented yet on MDN, but you should be able to understand them from the schema or tests in the second patch.

Emma or Sam, can you please check if the API addition is enough and appropriate for the Screenshots usecase, ideally in the next week, so that we could document and announce changes to addon devs?

Flags: needinfo?(sfoster)
Flags: needinfo?(emalysz)
Fission Milestone: --- → M7

The only issue I see is the lack of a bgColor option. Screenshots currently calls drawWindow(window, left, top, width, height, "#fff");. See shooter.js.

Is a background-color implicit in captureTab or might we get transparent screenshots without? Otherwise it looks like this should work.

The canvas context we use to call drawWindow is pretty much just used to get a data-uri from. In some cases we also use it to re-encode as JPEG if some capture size threshold is exceeded. We would have to either re-work that or (my preference) remove that step - see https://bugzilla.mozilla.org/show_bug.cgi?id=1649915#c6. - but I don't think that is relevant from the point of view of the extension api.

Flags: needinfo?(tomica)
Flags: needinfo?(sfoster)
Flags: needinfo?(emalysz)

(In reply to Sam Foster [:sfoster] (he/him) from comment #1)

The only issue I see is the lack of a bgColor option. Screenshots currently calls drawWindow(window, left, top, width, height, "#fff");. See shooter.js.

Is a background-color implicit in captureTab or might we get transparent screenshots without? Otherwise it looks like this should work.

Yeah, we have a hard-coded opaque white background, don't think there's need to enable changing that for now:
https://searchfox.org/mozilla-central/rev/f4b4008f5e/toolkit/components/extensions/parent/ext-tabs-base.js#136

The canvas context we use to call drawWindow is pretty much just used to get a data-uri from. In some cases we also use it to re-encode as JPEG if some capture size threshold is exceeded. We would have to either re-work that or (my preference) remove that step - see https://bugzilla.mozilla.org/show_bug.cgi?id=1649915#c6. - but I don't think that is relevant from the point of view of the extension api.

Right, you should be able avoid using a canvas from inside the page at all.

Flags: needinfo?(tomica)

Hi Emma, this is a Screenshots extension bug that will need to be fixed soon to be compatible with Fission. You can enabled Fission (Site Isolation) in Firefox Nightly's "Nightly Experiments" preferences: about:preferences#experimental

btw, the Module Owner wiki lists you as the triage owner of the "Firefox :: Screenshots" component, but Bugzilla still lists Ian Bicking as the triage owner.

Fission Milestone: M7 → M6c
Flags: needinfo?(emalysz)
c91bfe10583b076ed3a82bd91a17b647?d=mm&size=64
Assignee

Updated

29 days ago
Severity: -- → S2
Flags: needinfo?(emalysz)
Priority: -- → P1
c91bfe10583b076ed3a82bd91a17b647?d=mm&size=64
Assignee

Updated

29 days ago
Assignee: nobody → emalysz
You need to log in before you can comment on or make changes to this bug.

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK