459244 - SetWindow called on every paint when supporting CoreGraphics

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Currently, in Firefox 3.0.X and 3.X, SetWindow is called on every
paint in plugins that support the CoreGraphics drawing model.  This is
unnecessary, and (as it can lead to every SetWindow call being called
twice with exactly the same parameters) may cause confusion in
plugins.

Here's a way to see this bug in action:

1) Copy the latest version of the DebugEventsPlugin (from bug 441880)
   to /Library/Internet Plug-Ins.

2) Load the plugin distro's test.html into the browser (Firefox 2 or
   Firefox 3).  (By default the DebugEventsPlugin supports the
   CoreGraphics drawing model if the browser supports it.  Firefox 3
   supports the CoreGraphics drawing model.  Firefox 2 only supports
   the QuickDraw drawing model.)

3) Make the browser window smaller (vertically) than the plugin
   display (so that a vertical scrollbar appears).

4) Click once on the scrollbar's up or down arrow, and observe the
   calls to NPP_SetWindow() logged to the system console.

   In Firefox 2 you'll see two calls to NPP_SetWindow().  The first
   call's "window"'s coordinates accurately describe the dimensions
   and location of the whole (unclipped) plugin before the window is
   scrolled, but its clipRect is "empty" (it has zero width and
   height).  The second call's "window"'s coordinates accurately
   describe the whole (unclipped) plugin's dimensions and location
   after the window has been scrolled, and its clipRect accurately
   describes the part of it that's actually displayed (i.e. it's not
   "empty").

   In Firefox 3 you'll see four calls to NPP_SetWindow().  The first
   two are like Firefox 2's first call to NPP_SetWindow().  The third
   and fourth are like Firefox 2's second call to NPP_SetWindow().

   Firefox 3 will be have like Firefox 2 if you make the
   DebugEventsPlugin support the QuickDraw drawing model when loaded
   in Firefox 3.  To do this, add 'supportCoreGraphics="false"' to the
   <embed> tag that loads the DebugEventsPlugin.

In my next comment I'll attach a patch that fixes this bug.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

A comment indicates that [e-mail] (Matthew Gregan) already'd
noticed this problem.  My fix just (more or less) follows his
suggestion.

Here's a tryserver build made with this patch:

https://build.mozilla.org/tryserver-builds/2008-10-09_09:[e-mail]-bugzilla459244/[e-mail]

Comment 2

[Steven Michaud [:smichaud] (Retired)]

A fix for this bug may be needed to fix bug 450783.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 342446 [details] [diff] [review]
Fix

Looks good, but I'm a little worried that this may cause a recurrence of bug 419187.  We used to assert that the CGContext had not changed (effectively using the same logic you're using to decide whether to call SetCGContextChanged), but the reporters of that bug did not mention seeing the assertion fire.

Unfortunately the steps for reproducing that bug are not particularly reliable--bug 419187 comment #3 has the details.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Thanks, Matthew, for the pointer to bug 419187.

As it happens, I can easily reproduce the STR at bug 419187 comment #3
with a sufficiently old version of 1.9.0-branch Camino, and don't see
the problem in more recent nightlies.

So it's easy for me to tell that this patch does regress bug 419187 on
Camino :-(

I'll need to see what I can come up with.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix dumb mistake)

It was my dumb mistake that broke Camino.  I should have noticed that
(in FixUpPluginWindow()) mCGContextChanged could be reset without
SetWindow() ever having been called.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my rev1 patch:

https://build.mozilla.org/tryserver-builds/2008-10-14_14:[e-mail]-bugzilla459244-rev1/[e-mail]

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central (changeset f870582d4cdd).

Comment 8

[Daniel Veditz [:dveditz]]

Not sure the gain is worth the risk for stable releases. Is this a huge problem affecting lots of users?

Comment 9

[Daniel Veditz [:dveditz]]

Which is not to say we wouldn't approve a patch if you can justify its inclusion (and include the regression fixes in the branch patch). But it's not looking like something we'd take without a lot of justifying.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

This patch doesn't matter so much.  But the patch for bug 459530 might.  If the patch for bug 459530 lands on the 1.9.0 branch, this patch needs to land with it.

I don't currently know of a pressing need for the bug 459530 patch on the 1.9.0 branch.  But that might change.