870105 - Resolve BackgroundPageThumbs's use of nsIDOMWindowUtils.setCSSViewport

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

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.

Comment 1

[Drew Willcoxon :adw]

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?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Drew Willcoxon :adw]

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?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 5

[Drew Willcoxon :adw]

Attachment: size via browser.style

Hey, that does work.  Thanks, Tim.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 7

[Drew Willcoxon :adw]

Sure, changed in the landed patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c001ba8508a

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Drew Willcoxon :adw]

Here's the change we talked about on IRC, using nsIScreen.GetRectDisplayPix instead:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b027f7b8671d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c001ba8508a
https://hg.mozilla.org/mozilla-central/rev/b027f7b8671d