Bug 37194 - Prevent wrong use of PrintContext
Summary: Prevent wrong use of PrintContext
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-04-07 01:49 PDT by Shinichiro Hamaji
Modified: 2010-04-09 16:43 PDT (History)
2 users (show)

See Also:

Patch v1 (2.18 KB, patch)
2010-04-07 01:51 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (2.20 KB, patch)
2010-04-07 02:01 PDT, Shinichiro Hamaji
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2010-04-07 01:49:53 PDT
We can misuse PrintContext in several ways. We can call PrintContext::begin() twice, we can call PrintContext::end() without PrintContext::begin(), and we can forget PrintContext::end() after PrintContext::begin(). Bug 36049 is an example of such mistakes.

Let's add a boolean flag to prevent such misuses.
Comment 1 Shinichiro Hamaji 2010-04-07 01:51:31 PDT
Created attachment 52714 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2010-04-07 01:59:45 PDT
Comment on attachment 52714 [details]
Patch v1

>  PrintContext::~PrintContext()
>  {
> +    if (m_isPrinting)
> +        end();
>      m_pageRects.clear();
>  }

Or, we may want to add ASSERT(!m_isPrinting) here. I personally like this kind of automatic release, but I'm not sure if this is suitable for this case. If a user of PrintContext relies on this, her/his code may have begin() without end(), which might look weird a bit. Hmm... I changed my mind while I'm writing this comment. I'll revise the patch.
Comment 3 Shinichiro Hamaji 2010-04-07 02:01:56 PDT
Created attachment 52716 [details]
Patch v2
Comment 4 Eric Seidel (no email) 2010-04-09 14:03:13 PDT
Comment on attachment 52716 [details]
Patch v2

     if (m_isPrinting)
 43         end();
isn't needed.  the ASSERT is enough.
Comment 5 Eric Seidel (no email) 2010-04-09 14:04:01 PDT
Comment on attachment 52716 [details]
Patch v2

You should consider documenting what m_isPrinting is for in teh header, otherwise this looks OK (- the release-only auto-end stuff).

r=me with the two above comments addressed.
Comment 6 Yuzo Fujishima 2010-04-09 14:13:12 PDT
Why is m_isPrinting protected rather than private?
Comment 7 Shinichiro Hamaji 2010-04-09 16:41:04 PDT
Committed r57384: <http://trac.webkit.org/changeset/57384>
Comment 8 Shinichiro Hamaji 2010-04-09 16:43:00 PDT
Thanks for your suggestions! I addressed the three (2 from eric + 1 from yuzo) comments and landed the patch.