Resolve BackgroundPageThumbs's use of nsIDOMWindowUtils.setCSSViewport

RESOLVED FIXED in mozilla24

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

BackgroundPageThumbs's content script, backgroundPageThumbsContent.js, calls nsIDOMWindowUtils.setCSSViewport to resize the viewport used to capture thumbnails.  It calls it on every page load because I found that the viewport would automatically reset to a small size on page load.  Tim thought that calling it every page load was unnecessarily expensive.

So, find out whether it is expensive, and if it is, whether there's a better way to use setCSSViewport so it doesn't need to be called every page load.  (Or just assume it's expensive, if finding out whether there's a better way to use setCSSViewport is easier to do than finding out whether it's expensive.)

Or, find a better way to resize the viewport/browser altogether.  I tried other obvious things but setCSSViewport on every page load was the only one that worked.
roc, you reviewed patches in bug 590294, which added setCSSViewport.  We're using setCSSViewport in a content script of a remote browser to resize the viewport to an appropriate size before capturing the window to a canvas.  The concern is that we're doing this after every page load, which may be unnecessarily expensive, because the viewport is reset to an inappropriate size after page load.  Ideally we could do it only once and have it stick.  Is that not possible?  Or is there some other method you know of that we should be using to resize the viewport for capturing page screenshots in a remote browser?
Flags: needinfo?(roc)
I don't know what's going on here. Assuming this is on desktop, you shouldn't need to set the viewport, you should just be able to size your <browser> element appropriately and that should stick.
Flags: needinfo?(roc)
Unfortunately that doesn't work.  The resulting screenshot is empty and transparent.  Does it matter that in addition to being remote, the browser is in the private hidden window?
(In reply to Drew Willcoxon :adw from comment #3)
> Unfortunately that doesn't work.  The resulting screenshot is empty and
> transparent.  Does it matter that in addition to being remote, the browser
> is in the private hidden window?

Did you try setting the style directly? Like browser.style.width="200px" instead of browser.setAttribute("width",200)? The latter didn't work for me for newtab page preloading but the first one worked well.
Created attachment 758048 [details] [diff] [review]
size via browser.style

Hey, that does work.  Thanks, Tim.
Attachment #758048 - Flags: review?(ttaubert)
Comment on attachment 758048 [details] [diff] [review]
size via browser.style

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

It's always great to remove code! :)

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +139,5 @@
> +      getService(Ci.nsIScreenManager).
> +      primaryScreen.
> +      GetRect({}, {}, width, height);
> +    browser.style.width = (width.value / 3) + "px";
> +    browser.style.height = (height.value / 3) + "px";

I just realized that this does the same as PageThumbs._getThumbnailSize(). Can we just call that to not repeat ourselves?
Attachment #758048 - Flags: review?(ttaubert) → review+
Sure, changed in the landed patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c001ba8508a
Assignee: nobody → adw
Status: NEW → ASSIGNED
Hmm. It just came to my mind that having the loading browser be as big as the target thumbnail size is probably not what we want, is it? What about having a static size like 1024x768 or higher so that we don't break the layout of web pages? It seems like 1/9th of the screen size will render very strange thumbnails for most of the popular sites.
Here's the change we talked about on IRC, using nsIScreen.GetRectDisplayPix instead:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b027f7b8671d
https://hg.mozilla.org/mozilla-central/rev/9c001ba8508a
https://hg.mozilla.org/mozilla-central/rev/b027f7b8671d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Updated

5 years ago
Depends on: 908060
You need to log in before you can comment on or make changes to this bug.